Repository navigation
Clarify internal programming style #49567
Description
Every so often someone will submit a pull request that is the result of running rustfmt on a particular file, or set of files (examples: #49360, #38567, #30684).
Presumably we want to encourage fixing mistakes in the code style, though at the moment rustfmt can produce undesirable changes (e.g. see the comments in #37349). Without guidelines, these can either be merged directly, or it left up to the individual reviewer to decide which changes are preferred.
It seems to me it would be better to do one of two things:
- Add
rustfmttotidyand ensure code always adheres to one style. - Explicate style guidelines, and then only allow PRs fixing mistakes that haven't been spotted from earlier changes (for example, if
rustfmtis too strict or not good enough). I.e. not changing style choices that were the original intention of the author if they're not specified by the style guide.
At the moment, I think style changes are being made, through blind use of rustfmt, that leave the code in a worse state than before. I think it'd be preferable if we could avoid this; does anyone else have thoughts?
Activity
steveklabnik commentedon Apr 2, 2018
The intention has always been that, when
rustfmtis ready, projects by the Rust team will userustfmtwith no configuration for its projects. However,rustfmtisn't ready yet, so we haven't gotten there yet.Generally, discussions are best had on https://internals.rust-lang.org/ rather than the issue tracker; I'll leave @rust-lang/compiler to decide if this should be moved there, though.
varkor commentedon Apr 2, 2018
In this case, it would seem sensible to discourage "Run rustfmt on [X]"-style PRs in general.
Ah, that's true. I was thinking of it more as a "contribution documentation" omission (i.e. that this should be made clearer somewhere in the repository), but more nuanced discussions (if anyone would be opposed to stating that these kinds of PRs are discouraged until
rustfmtbetter fitsrustc's use cases) would best be taken elsewhere.steveklabnik commentedon Apr 2, 2018
Historically, this has been helpful to point out issues in
rustfmtto get it to be good enough to get to 1.0 :)Yes, this is exactly why I didn't close it; if the team wants to document some kind of policy, this is totally a bug, but if there's more disagreement about the policy than I'm aware of (as I'm not on the team) and it needs to be discussed before documenting it, then it'd better be on internals. We'll see!
ehuss commentedon Apr 2, 2018
Cargo recently attempted this, but just today it was disabled (rust-lang/cargo#5278). It can cause friction with PRs since most people aren't used to running rustfmt all the time.
oli-obk commentedon Apr 2, 2018
Clippy tried it a while back, but we disabled it out of the same reason that cargo did so.
Options that I see:
nrc commentedon Apr 4, 2018
We might also be able to solve this on the rustfmt side by pinning to a version number.
nrc commentedon Apr 4, 2018
At this stage I think that rustfmt formatting is stable enough that re-formatting crates is unlikely to cause much churn, therefore it is fine to land formatting PRs. However, I wouldn't go so far as to encourage it because reviewing and landing them can be a pain.
nikomatsakis commentedon Oct 30, 2018
Closing in favor of discussing on internals