Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept more types in style! macro #273

Merged
merged 10 commits into from
Nov 3, 2019
Merged

Accept more types in style! macro #273

merged 10 commits into from
Nov 3, 2019

Conversation

MuhannadAlrusayni
Copy link
Contributor

PR for #268, please give extra review since this is my 1st PR :D

@MartinKavik
Copy link
Member

I don't know why, but GitHub doesn't show tests for this PR - https://travis-ci.org/David-OConnor/seed/pull_requests

Copy link
Contributor Author

@MuhannadAlrusayni MuhannadAlrusayni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it show up now

@MuhannadAlrusayni
Copy link
Contributor Author

MuhannadAlrusayni commented Oct 31, 2019

Are there any thing left to be fixed ?

@David-OConnor
Copy link
Member

David-OConnor commented Nov 1, 2019

I'm adding your test code as a demo of this to the guide.

@MartinKavik
Copy link
Member

Are there any thing left to be fixed ?

I've added some comments for final polishing. Once resolved, we can merge it. Good job!

@MuhannadAlrusayni
Copy link
Contributor Author

Waiting your review :D

@MartinKavik
Copy link
Member

@MuhannadAlrusayni If you enjoyed writing this PR I recommend you to look at AtValue - there is almost the same problem but it's a little bit more complicated because there are more variants :)

@MartinKavik
Copy link
Member

It seems that unformatted code has been merged into master from another PR. @MuhannadAlrusayni could you fix it? Run cargo make verify and you should see some differences. If you don't see anything try to rebase your changes onto the current master and run cargo make verify again. Or let me know and I'll fix it.

@David-OConnor: could you add requirement that we can merge only PR with successful tests? Or at least add me to the list for travis notifications (if it's doable) so I can fix it / send PR with fix as soon as possible? build:failing isn't nice in the readme and it creates complications like this one.

@MuhannadAlrusayni
Copy link
Contributor Author

I'll rebase and see if I can fix it.

@David-OConnor
Copy link
Member

@MartinKavik Done.

@MuhannadAlrusayni
Copy link
Contributor Author

I rebase and force push, not sure if I did it correctly, since this is my first time doing rebase :D.

@MartinKavik
Copy link
Member

MartinKavik commented Nov 3, 2019

@MuhannadAlrusayni I just run cargo make verify on your master and created PR https://github.com/MuhannadAlrusayni/seed/pull/1 for you, maybe you and previous contributor have older Rust or Clippy or Fmt, I don't know exactly. Or you forgot to run cargo make verify or there are other problems, who knows.

OT:

my first time doing rebase

It's just like playing Solitaire if you have the right tools.. just a few drag & drops and clicks.. https://www.youtube.com/watch?v=nAMbLbgxriI :)

@MuhannadAlrusayni
Copy link
Contributor Author

MuhannadAlrusayni commented Nov 3, 2019

looks like this come from c5e39c6#diff-ce655e9c982a8e83550dccb0b7a646f2R231

And I believe the reason that it was not fixed is that when I forked Seed repo this commit was not pushed yet to seed/master, then when I finished my commits I checked with cargo make verify things was fine but when I re-base I didn't re-run cargo make verify since I was looking at the diffs from my master and seed/master there was not conflict so I pushed to my master.

now when I run cargo make verify this lint you fix got fixed automatically.

Anyway I merged your PR, now every thing should be in sync.

if you have the right tools..

Sure I use Magit here, but I don't have much experience with git yet :D.

If you enjoyed writing this PR I recommend you to look at AtValue - there is almost the same problem but it's a little bit more complicated because there are more variants :)

Sure that was great experience, I'll contribute when I have time.

@MartinKavik
Copy link
Member

@David-OConnor Merge it, please. It'll also fix master.

@David-OConnor David-OConnor merged commit 44a60e2 into seed-rs:master Nov 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants