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

rustfmt'ed #1390

Merged
merged 1 commit into from
Apr 13, 2018
Merged

rustfmt'ed #1390

merged 1 commit into from
Apr 13, 2018

Conversation

cyplo
Copy link
Contributor

@cyplo cyplo commented Apr 8, 2018

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

@cyplo cyplo force-pushed the rustfmt branch 2 times, most recently from 1a85638 to ff78801 Compare April 8, 2018 13:27
@cyplo cyplo changed the title Obey rustfmt rustfmt'ed Apr 8, 2018
@cyplo
Copy link
Contributor Author

cyplo commented Apr 8, 2018

cc @nrc

Copy link
Member

@nrc nrc left a 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.

@cyplo
Copy link
Contributor Author

cyplo commented Apr 9, 2018

Would pinning a version of rustfmt to use on CI help ? Diesel is doing something along those lines, if I recall correctly.

@nrc
Copy link
Member

nrc commented Apr 10, 2018

Would pinning a version of rustfmt to use on CI help ?

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.

@cyplo
Copy link
Contributor Author

cyplo commented Apr 10, 2018

That's a very good point, I haven't thought of that.
How about either merging just the formatting changes and then the Travis change we would have stored somewhere near some issue in the tracker, for later use?
OR
I could allow this separate build job that checks the fmt compliance to fail, i.e. mark as allow failures on Travis?

What do you think? Which one sounds better? Thanks!

@nrc
Copy link
Member

nrc commented Apr 10, 2018

I would just land the formatting changes and hope that we can make the CI better in the future with some help from Rustfmt.

@bors
Copy link
Contributor

bors commented Apr 10, 2018

☔ The latest upstream changes (presumably #1388) made this pull request unmergeable. Please resolve the merge conflicts.

@cyplo
Copy link
Contributor Author

cyplo commented Apr 10, 2018

Alrighty, will catchup with master and remove the travis changes and ping you when done. thank you :)

Reformatted the code using cargo fmt.
@cyplo
Copy link
Contributor Author

cyplo commented Apr 10, 2018

Cool, the changes are now formatting changes only, removed the travis changes altogether from the PR
cc @nrc

@nrc nrc merged commit 6e03f38 into rust-lang:master Apr 13, 2018
@nrc
Copy link
Member

nrc commented Apr 13, 2018

Thanks!

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