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

Don’t rustfmt, yet #336

Closed
lucab opened this issue May 12, 2017 · 3 comments
Closed

Don’t rustfmt, yet #336

lucab opened this issue May 12, 2017 · 3 comments

Comments

@lucab
Copy link

lucab commented May 12, 2017

Capturing my previous comment from #326 (comment).

It looks like a cargo fmt run on this crate produces a very large delta (~1K lines). Auto-formatting hooks are part of many people workflow (including mine, so I'm biased) to avoid stepping into style-bikeshedding. It would be nice to have a global rustfmt pass in here to avoid seeing such large spurious deltas in further PRs, especially from newcomers.

@SimonSapin
Copy link
Member

TL;DR Please don’t, yet.

I love the idea of rustfmt, but I think it’s not ready yet.

Every week, This Week in Rust shows significant activity in style RFCs. This means that the preferred style is not stable yet and, assuming that rustfmt is kept up to date with it, the output of rustfmt on unchanging code will keep changing in the coming months.

Ideally we’d run it once and have one disruptive PR, and after that enforce on Travis-CI that further code changes still follow the same style by running rustfmt and failing (rejecting a PR) if it changes anything. This doesn’t work if rustfmt itself keeps changing its behavior.

Also, the initial diff of #326 included a lot of changes that I feel are undesirable. Last I heard, rustfmt has lots of configuration options. Once rustfmt is stable I can go and tweak all these knobs until I find a configuration that feels less undesirable, but I’d prefer not to get into that just yet.

Auto-formatting hooks are part of many people workflow […] have a global rustfmt pass in here to avoid seeing such large spurious deltas in further PRs

Disagree. The onus is on you to have a quick glance at your diffs before making a commit and before submitting a PR, to avoid accidentally submitting large changes unrelated to those you intended.

Unless you’re certain that every project you’re ever gonna work on have the same formatting conventions / preferences, configuring your environment to auto-format globally rather than on a per-project basis is not appropriate.

@SimonSapin SimonSapin changed the title rustfmt all the things? Don’t rustfmt, yet May 12, 2017
@lucab
Copy link
Author

lucab commented May 12, 2017

That's why I asked, and why I said I'm biased (and partially spoiled by gofmt) 😄

Thanks for the quick feedback and for the detailed answer, I'll leave this open to make it easier for other random contributors to spot!

@SimonSapin
Copy link
Member

rustfmt has come a long way in the last 2+ years.

The master branch now had a manual run of rustfmt, but formatting is not enforced on CI at the moment.

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

2 participants