Skip to content

Run Rustfmt #111

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

Merged
merged 1 commit into from
Sep 23, 2019
Merged

Run Rustfmt #111

merged 1 commit into from
Sep 23, 2019

Conversation

josephlr
Copy link
Member

@josephlr josephlr commented Sep 22, 2019

#104 contained rustfmt errors, but we missed them because formatting errors don't (currently) fail the CI.

This PR fixes the formatting errors

@dhardy
Copy link
Member

dhardy commented Sep 22, 2019

See #39 and #23. Any reason we should revisit the decision to allow_failures now?

@josephlr
Copy link
Member Author

See #39 and #23. Any reason we should revisit the decision to allow_failures now?

The main reason is that we have it in the CI, but allowing it to fail means we don’t notice it (like in #104) , so it gets missed. For me, the issue is that I autoformat the code as I write it, so I end up with unrelated changes when I edit a misformatted file.

On my end, the issue isn’t that bad, but it’s also not a huge deal to require PRs to just run cargo fmt. I know @newpavlov brought up a (valid) point about stability, but that’s virtually always an issue with the nightly builds of rustfmt (in that they sometimes fail, breaking the CI), but we use stable so that particular issue won’t affect us.

Thoughts?

@dhardy
Copy link
Member

dhardy commented Sep 22, 2019

You could get used to typing cargo fmt and committing the result before making any other changes. A pain I know, but any way about it, half the devs using rustfmt and the other not is a pain for someone.

But since you've been doing far more work on this crate than me I'll give you priority on this, if @newpavlov agrees.

@newpavlov
Copy link
Member

In my experience it's PITA to deal with rustfmt failures in CI, especially for outside contributors. And not everyone (e.g. me) uses stable toolchain for development. While I can live with the proposed change, I think fixing formatting from time to time (e.g. as part of release process) is a slightly better approach.

@dhardy
Copy link
Member

dhardy commented Sep 23, 2019

Then the decision seems clear: we continue to allow failures for rustfmt.

Rebase without that part @josephlr.

@josephlr josephlr changed the title Run Rustfmt and make formatting error fail the CI Run Rustfmt Sep 23, 2019
@josephlr
Copy link
Member Author

Rebase without that part @josephlr.

Done, updated title and description.

Thanks for the clarification @newpavlov

@dhardy dhardy merged commit 6bd6e04 into rust-random:master Sep 23, 2019
@josephlr josephlr deleted the fmt branch September 23, 2019 20:43
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