-
Notifications
You must be signed in to change notification settings - Fork 195
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
Support Rustfmt #23
Conversation
60f6cc7
to
26af4ae
Compare
Does this actually help anyone? I find it needless fiddling, debatable whether the result is an improvement, and potentially clashing with other PRs. |
@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,
|
As for clashing with other PRs, there's really only #14 and it's simple enough to just run Rustfmt on that PR. |
In my book, "properly" means simply reasonably neat and consistent. Actually, I may be biased because of bad experiences with
I don't count this very important (it's very easy to modify a line in insignificant ways).
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. |
I also had a bad experience with 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. |
I think it will be better use |
Are the EDIT: I figured it out the tests are now running normally. |
This should be ready to merge. The |
We will actually run
rustfmt
in a later PR