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

Support Rustfmt #23

Merged
merged 1 commit into from
Jun 10, 2019
Merged

Support Rustfmt #23

merged 1 commit into from
Jun 10, 2019

Conversation

josephlr
Copy link
Member

@josephlr josephlr commented Jun 10, 2019

We will actually run rustfmt in a later PR

@josephlr josephlr force-pushed the rustfmt branch 2 times, most recently from 60f6cc7 to 26af4ae Compare June 10, 2019 05:43
@dhardy
Copy link
Member

dhardy commented Jun 10, 2019

Does this actually help anyone? I find it needless fiddling, debatable whether the result is an improvement, and potentially clashing with other PRs.

@josephlr
Copy link
Member Author

@dhardy ideally there would be a way to just make sure that new PRs were properly formatted. That way the git blame wouldn't get messed up by switching over to this. However, rustfmt doesn't support such a thing IIRC. So the options are either:

  • Not having any formatting tools for PRs
  • This change (or something equivalent)

@josephlr
Copy link
Member Author

As for clashing with other PRs, there's really only #14 and it's simple enough to just run Rustfmt on that PR.

@dhardy
Copy link
Member

dhardy commented Jun 10, 2019

make sure that new PRs were properly formatted

In my book, "properly" means simply reasonably neat and consistent. Actually, I may be biased because of bad experiences with rustfmt in the past (frequently finding I have the wrong version, and it being impossible to work with old commits without bumping into formatting issues).

That way the git blame wouldn't get messed up

I don't count this very important (it's very easy to modify a line in insignificant ways).

As for clashing with other PRs

Sorry, this isn't an issue here.

I'll let @newpavlov decide since he's mostly responsible for this repo, but in my book having to do extra work to get PRs merged for very little gain is just not worth it.

@newpavlov
Copy link
Member

I also had a bad experience with rustfmt, but I think consistent formatting is not a bad thing to have. (though personally I dislike some defaults) But I believe rustfmt failure should be allowed in CI, as otherwise it's quite irritating for contributors. We can run it manually from time to time.

I think it will be better to revert formatting changes in this PR and postpone them until other PRs get sorted out. In other words let's limit it to CI changes.

@josephlr
Copy link
Member Author

I think it will be better to revert formatting changes in this PR and postpone them until other PRs get sorted out. In other words let's limit it to CI changes.

Done, the check now prints the diff to the Travis CI log, but will not fail the build. I'll do a separate CL to actually run rustfmt once all the other PR are merged/closed.

@newpavlov
Copy link
Member

I think it will be better use allow_failures for rustfmt test (it will make it stand out more on CI page) and remove cargo tests (they are simply redundant).

@josephlr
Copy link
Member Author

josephlr commented Jun 10, 2019

I think it will be better use allow_failures for rustfmt test (it will make it stand out more on CI page) and remove cargo tests (they are simply redundant).

Are the cargo tests redundant? I thought they were needed to make sure the tests pass on stable w/ Linux.

EDIT: I figured it out the tests are now running normally.

@josephlr
Copy link
Member Author

This should be ready to merge. The rustfmt job fails, but the build as a whole succeeds.

@newpavlov newpavlov merged commit ce4a089 into rust-random:master Jun 10, 2019
@josephlr josephlr deleted the rustfmt branch June 11, 2019 00:52
@dhardy dhardy mentioned this pull request Sep 22, 2019
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