-
Notifications
You must be signed in to change notification settings - Fork 894
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
rustfmt'ed #1390
rustfmt'ed #1390
Conversation
1a85638
to
ff78801
Compare
cc @nrc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great thank you! I've wanted to run rustfmt over Rustup recently and I think we should land this. However, we've found that running Rustfmt on CI is not a great experience yet because as Rustfmt evolves, it's formatting changes and can cause tests to fail. We're working on this on the Rustfmt side, but in the meantime I think it is better to land the formatting changes without the CI change.
Would pinning a version of rustfmt to use on CI help ? Diesel is doing something along those lines, if I recall correctly. |
Yeah, I think this is a good stop-gap. The problem with this is that it makes it difficult for contributors to get the right version of Rustfmt - I think they have to build from source to get the right version, and this might not work if it no longer compiles with their current toolchain. |
That's a very good point, I haven't thought of that. What do you think? Which one sounds better? Thanks! |
I would just land the formatting changes and hope that we can make the CI better in the future with some help from Rustfmt. |
☔ The latest upstream changes (presumably #1388) made this pull request unmergeable. Please resolve the merge conflicts. |
Alrighty, will catchup with master and remove the travis changes and ping you when done. thank you :) |
Reformatted the code using cargo fmt.
Cool, the changes are now formatting changes only, removed the travis changes altogether from the PR |
Thanks! |
Reformatted the code using cargo fmt.
Added a separate Travis job to the build matrix that fails when code is
not properly formatted
potentially closes #1291 as well