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

Make Travis fail non-formatted builds #5356

Closed
djc opened this issue Apr 13, 2018 · 9 comments
Closed

Make Travis fail non-formatted builds #5356

djc opened this issue Apr 13, 2018 · 9 comments

Comments

@djc
Copy link
Contributor

djc commented Apr 13, 2018

It feels like the cargo fmt setup we have is a bit half-assed: on the one hand, we test for it in CI; on the other, stuff regularly gets merged without cargo formatting. I think #5355 is the third time this week that I had to clean up other people's stuff. It feels to me like we should either enforce it or let it go; having contributors deal with other people's formatting issues is not so nice.

@matklad
Copy link
Member

matklad commented Apr 13, 2018

We had it once, but then removed it: #5278

  1. it creates more friction for new contributors. It would be awesome if we could share some kind of pre-commit hook for this, but they are not sharable

  2. it complicate back porting, because rustfmt slightly changes formatting with new versions.

@djc
Copy link
Contributor Author

djc commented Apr 13, 2018

So does (2) mean we don't want to do much reformatting at all? It would be nice maybe if bors/homu gave friendly feedback about it in PRs that don't conform.

@matklad
Copy link
Member

matklad commented Apr 13, 2018

The plan is to just run rustmt manually once in a while.

@dwijnand
Copy link
Member

here's another idea (for this and other non-functional aspects of code, like performance) is, instead of blocking a PR, check on master only or nightly, and raise an alert (e.g a chat message or open an issue).

@dwijnand
Copy link
Member

rustfmt slightly changes formatting with new versions

another option is to pin the rust version in CI instead of using a moving channel, like https://github.com/assert-rs/assert_cli/blob/d3daabbcef3bb1323af5cf71e40a0d4ee16f7647/.travis.yml#L17-L22. wdyt?

@dwijnand
Copy link
Member

Ashley shared a post on how to enforce style in CI: https://medium.com/@ag_dubs/enforcing-style-in-ci-for-rust-projects-18f6b09ec69d

With Rustfmt reaching 1.0 RC1 (https://www.ncameron.org/blog/rustfmt-1-rc/), do we want to experiment with this at this time?

Another possible option is look at integrating with GitHub Checks (https://blog.github.com/2018-05-07-introducing-checks-api/)? So PRs have the info about where style has deviated, but without having it fail CI.

@dwijnand
Copy link
Member

https://crate-ci.github.io/ (which seems like a great resource) suggests in https://crate-ci.github.io/pr/rustfmt.html#specifying-your-style to lock down the project's style with a .rustfmt.toml. Would that alleviate back-porting concerns?

(By the way, the modern command to run is rustfmt --print-config default .rustfmt.toml)

@dwijnand
Copy link
Member

I think we should just close this. For reason mentioned above there's no interest to do this (to the best of my knowledge).

@dwijnand
Copy link
Member

dwijnand commented Dec 6, 2018

Always happy to change this decision, perhaps in the face of Rust 2018 rustfmt 😄

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

No branches or pull requests

3 participants