Skip to content

Tracking issue: check write mode follow-up #2536

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

Closed
5 of 10 tasks
nrc opened this issue Mar 16, 2018 · 5 comments
Closed
5 of 10 tasks

Tracking issue: check write mode follow-up #2536

nrc opened this issue Mar 16, 2018 · 5 comments

Comments

@nrc
Copy link
Member

nrc commented Mar 16, 2018

Described in #1977 (comment) This would be used mainly by CI and would emit exit code 0 if no formatting was necessary and 1 if formatting is necessary. The output to stdout would be the same as diff mode.

Mentoring instructions:

Implementation

  • add check to the rustfmt bin options
  • implement check like diff mode, but with different exit codes (will require refactoring or exit code handling, I think)
  • document check mode
  • add a test - I think this would have to be an ad hoc unit test in the binary.

Follow ups

  • change exit codes as described in Audit exit codes #1977 (comment) (it might be easier to do this whilst refactoring the exit code handling, since I think it will lead to some code being removed).
  • make the newline character in diffs optional (i.e., it requires a command line arg)
  • use check mode for our tests (I'm not sure if this is possible, but might work)
  • use check mode in the CI for an official Rust project - one of the RLS helper crates might be a good place to start. Document what is necessary in the Rustfmt repo.
  • align cargo fmt exit codes with rustfmt ones (this might be trivial once this issue is fixed)
  • implement/verify other parts of the proposal in Audit exit codes #1977 (comment)
@nrc nrc added good first issue Issues up for grabs, also good candidates for new rustfmt contributors p-high labels Mar 16, 2018
@toolness
Copy link

Hey, I'd like to help with this--should I just start working on a PR?

@toolness
Copy link

Oh whoops, never mind, just noticed #2539!

@davidbarsky
Copy link
Contributor

@toolness There are lot of low-hanging fruit in my PR! If you've got the time/bandwidth, I'd love to get feedback on some of my design decisions, particularly how I pass around WriteMode. It feels a bit messy, and I'm not too experienced in OSS.

(Apologies if I broke some social rule I'm unaware of...)

@nrc
Copy link
Member Author

nrc commented Mar 18, 2018

Hi @toolness thanks for your interest, but yeah it looks like someone already started on this. If you want another way to get stuck into rustfmt, #2531 might be good - it is not small or easy, but it doesn't require much knowledge of rustfmt - it is essentially refactoring to remove code dup, but in a non-trivial way. If that sounds interesting, I can provide instructions over on that issue.

@toolness
Copy link

Sounds good @davidbarsky! I still consider myself fairly inexperienced in Rust, but I'll take a look at your PR and see if there's any feedback I can provide.

@nrc, thanks for the tip about #2531, I just replied in that issue!

@nrc nrc mentioned this issue Apr 10, 2018
2 tasks
@nrc nrc removed good first issue Issues up for grabs, also good candidates for new rustfmt contributors p-high labels Apr 19, 2018
@nrc nrc added this to the 1.0 milestone Apr 19, 2018
@nrc nrc changed the title Implement a check write mode Tracking issue: check write mode follow-up Apr 19, 2018
@nrc nrc closed this as completed Apr 20, 2018
nrc added a commit that referenced this issue Apr 20, 2018
And don't print end of line characters by default in diffs

cc #2536
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

3 participants