Implemented rough draft of `check` write mode. #2539

davidbarsky · 2018-03-17T01:57:31Z

This is an attempt to address #2536. Of the items to handle in the list, this PR handles:

check to the rustfmt bin options
implement check like diff mode, but with different exit codes.

It does not, at press time:

document the check mode
add a test to make sure this works.

I'll add the latter two this weekend.

(This PR is somewhat clunky, as I was able to propagate WriteMode by adding an additional field to the Summary struct. Alternatives include returning a FmtResult<(Summary, Option<WriteMode>)>, but that felt messy.)

davidbarsky · 2018-03-18T19:58:00Z

In commit 533df58, I introduced a simple test case for --write-mode=check using assert-cli, but that introduces a new dependency, and I'm not sure that's a good decision. If introducing this dependency is okay, I'd happily write some additional test cases for the other WriteModes and/or introduce more testing for the CLI options.

Additionally, in commit b0ece6b5, I've documented the check option in the README.

I'm not too happy with how I'm handling propagation of WriteMode—I feel like I'm using the Summary struct as a de-facto context object. Would appreciate advice on handling this better.

nrc · 2018-03-18T21:38:50Z

src/bin/main.rs

@@ -342,6 +346,8 @@ fn main() {
        Ok(summary) => {
            if summary.has_operational_errors() {
                1
+            } else if summary.has_diff && summary.write_mode == Some(WriteMode::Check) {
+                1


It will probably be easier to change the other exit codes now, rather than later.

Alrighty! I'll implement the stuff suggested in this comment: #1977 (comment)

nrc · 2018-03-18T21:40:04Z

src/config/options.rs

@@ -183,6 +183,9 @@ configuration_option_enum! { WriteMode:
    Checkstyle,
    // Output the changed lines (for internal value only)
    Modified,
+    // Displays how much of the input was processed, but if anything was modified, rustfmt quits
+    // with exit code 0. This option is intended to be run as part of a CI pipeline.


I think the exit code is backwards - it should quit with 1 if anything was changed. It displays a diff, if there is one, not progress exactly.

D'oh—you're right. I'll fix in subsequent commits.

Fixed/resolved.

nrc · 2018-03-18T21:40:46Z

src/config/summary.rs

@@ -26,6 +28,9 @@ pub struct Summary {
    // Formatted code differs from existing code (write-mode diff only).
    pub has_diff: bool,

+    // What write mode rustfmt was invoked with, if any.
+    pub write_mode: Option<WriteMode>,


I don't think this needs to be optional - even if there is no explicit write mode, then there is a default.

Good point. I'll default it to overwrite?

Resolved, defaults to Overwrite.

nrc · 2018-03-18T21:45:57Z

README.md


 * `overwrite` Is the default and overwrites the original files _without_ creating backups.
 * `replace` Overwrites the original files after creating backups of the files.
 * `display` Will print the formatted files to stdout.
 * `plain` Also writes to stdout, but with no metadata.
 * `diff` Will print a diff between the original files and formatted files to stdout.
         Will also exit with an error code if there are any differences.
+* `check` Similar to `diff`, but is intended to be run in during CI and returns error code 1
+          on failure instead of 4 as `diff` does.  


I would document check mode in its own right, rather than comparing to diff.

Good point; will do.

nrc · 2018-03-18T21:47:51Z

Thanks for the PR! The 'hard' bit of running the new write mode looks great. I agree that it would be better to avoid adding the write mode to the summary and have suggested a possible solution in a review comment.

I think adding assert-cli as a dev-dependency is fine. Would be good to test a few of the exit code situations.

davidbarsky · 2018-03-18T22:35:51Z

Thanks for the PR! The 'hard' bit of running the new write mode looks great. I agree that it would be better to avoid adding the write mode to the summary and have suggested a possible solution in a review comment.

Thanks! Will address shortly.

I think adding assert-cli as a dev-dependency is fine. Would be good to test a few of the exit code situations.

Sounds good, I'll write test cases for those. Should I move the assert-cli tests away from main.rs, or...?

davidbarsky · 2018-03-19T01:15:51Z

So far:

Added better documentation for WriteMode::Check, both in --help and README.md.
Implemented Default for WriteMode, where default is Overwrite. This removes several Options in summary and main.

Still to do:

Refactor/simplify exit codes (I think the acceptance criteria for this PR should include this.)
Write assert-cli tests for the new exit code system.

topecongiro

Thank you the PR! I think the code handling exit code needs some update, but otherwise LGTM.

topecongiro · 2018-03-19T01:40:15Z

src/bin/main.rs

@@ -342,6 +346,8 @@ fn main() {
        Ok(summary) => {
            if summary.has_operational_errors() {
                1


This part is a bit wrong. IIRC in check mode, the exit code should be either 0 or 1, but this change does not seem to respect it.

Thanks for the feedback! I haven't gotten around to implementing #1977 (comment), but when I do later today, this the exit code handling should be far nicer and correct, wheras with my PR, it is not.

davidbarsky · 2018-03-20T21:08:42Z

Sorry for the delay; work happened. I correct the exit code handling as I understood #1977 (comment). I'd like to clarify—should I log cause with eprintln! for non-zero exit codes?

nrc · 2018-03-22T03:32:06Z

should I log cause with eprintln! for non-zero exit codes?

Yes, I think there should be some kind of error message. However, if we're emitting errors or warnings or whatever anyway, then I don't think we need to emit an additional message.

nrc · 2018-03-22T03:33:01Z

src/bin/main.rs

-            } else if summary.has_diff {
-                // should only happen in diff mode
-                4
+                1


You could use || rather than having multiple else if branches here.

Good point, will update later today.

nrc · 2018-03-22T03:40:38Z

src/bin/main.rs

+            if let Some(mode) = options.write_mode {
+                error_summary.add_write_mode(mode)
+            }
+


I thought I suggested a refactoring to avoid saving the write mode in the error summary, but maybe that was on another PR since I can't find any trace of it.

I think that by moving some code around in the main function so that we know the write mode (i.e., process the opts outside of execute), then we don't need to save it into the summary.

I thought I suggested a refactoring to avoid saving the write mode in the error summary, but maybe that was on another PR since I can't find any trace of it.

No worries.

I think that by moving some code around in the main function so that we know the write mode (i.e., process the opts outside of execute), then we don't need to save it into the summary.

I think that's a good idea. I wanted to introduce a minimally invasive change with this PR, but if exit codes are gonna change, so should this.

nrc · 2018-03-22T04:46:06Z

README.md


 * `overwrite` Is the default and overwrites the original files _without_ creating backups.
 * `replace` Overwrites the original files after creating backups of the files.
 * `display` Will print the formatted files to stdout.
 * `plain` Also writes to stdout, but with no metadata.
 * `diff` Will print a diff between the original files and formatted files to stdout.
         Will also exit with an error code if there are any differences.
+* `check` Checks if a diff can be generated. If so, rustfmt outputs a diff and quits with exit code 1.


I would re-word, something like: "Checks if the program's formatting matches what rustfmt would do. Silently exits with code 0 if so, emits a diff and exits with code 1 if not. This option is designed to be run in CI-like where a non-zero exit signifies incorrect formatting."

I like that; can do.

…rlier

davidbarsky · 2018-03-28T03:33:17Z

Sorry about the slowness on my end. I've been on-call at work, but that's over. With the coming holiday, I should be able to devote some time to landing this issue. Anyways:

I updated the README.md as per @nrc's suggestion.
Made the exit-code handling less bad.
Moved WriteMode out of the summary, handling parsing in fn main() {...}

That being said, the way I'm handling WriteMode isn't great. I think if I'm to refactor how exit code handling and argument parsing are handled, I'd like to use StructOpt, but I'd like to be sure that it's okay I introduce this dependency. Thoughts?

nrc · 2018-03-28T14:57:43Z

I'd like to use StructOpt, but I'd like to be sure that it's okay I introduce this dependency. Thoughts?

How does using StructOpt help? I'm kind of OK (I like StructOpt), but worry it might be churn for no benefit.

davidbarsky · 2018-03-31T02:29:40Z

How does using StructOpt help? I'm kind of OK (I like StructOpt), but worry it might be churn for no benefit.

I had some more time to think about it—it won't. Oops.

nrc · 2018-04-09T01:02:52Z

@davidbarsky How's it going on this? Got any questions I can answer?

davidbarsky · 2018-04-10T03:14:01Z

@nrc Hi Nick! Yes! Some guidance around changing fn execute. I'm thinking that, based off your suggestion above—quoted for reference:

I think that by moving some code around in the main function so that we know the write mode (i.e., process the opts outside of execute), then we don't need to save it into the summary.”

—fn execute can be broken into a function that handles the parsing of getopts/reading of configuration files, and another function than does the the actual formatting process. The results of the command/config parsing can threaded through main so that the result of the attempted run of rustfmt can be associated with the user's command config, thereby not overloading fn execute or Summary with extraneous information. Does this seem reasonable?

(apologies for the delay.)

nrc · 2018-04-10T09:34:01Z

Does this seem reasonable?

Yes, I think so

davidbarsky · 2018-04-19T02:40:40Z

Will need to rebase on mainline, but this I'm getting errors with rustc-ap-rustc_errors. Main thing—I'm not sure how I can make the handling of WriteMode/exit codes better even with refactoring. I'd like to avoid holding up further work/progress on this, I'll post what I've got. If this isn't what y'all are looking for, feel free to take this work and build atop of it/drop it.

# Conflicts: # Cargo.lock # src/bin/main.rs

davidbarsky · 2018-04-19T02:48:31Z

Okay, rebased. Should be conflict-free.

nrc · 2018-04-19T23:14:02Z

This looks great, thank you!

Update README.md to reflect change in #2539

Per rust-lang/rustfmt#2539 I think check and diff do the same thing, except that check now returns a nonzero exit code if there are changes that rustfmt would make.

Add rough draft of check mode. Not unit tested.

2b258f7

toolness mentioned this pull request Mar 17, 2018

Tracking issue: check write mode follow-up #2536

Closed

10 tasks

davidbarsky added 2 commits March 18, 2018 15:41

Added assert-cli; simple test case for --write-mode=check

533df58

Lightly documented check WriteMode

b0ece6b

nrc reviewed Mar 18, 2018

View reviewed changes

wrote clearer phrasing for config::options::WriteMode::Check

bda78f8

davidbarsky force-pushed the master branch from 594b464 to bda78f8 Compare March 19, 2018 00:10

Implemented default for WriteMode where default is Overwrite

d5944ee

topecongiro reviewed Mar 19, 2018

View reviewed changes

Simplified exit code handling

7412ff6

nrc reviewed Mar 22, 2018

View reviewed changes

nrc mentioned this pull request Mar 22, 2018

should_report_error: don't report error when using stdin #2495

Closed

davidbarsky added 3 commits March 25, 2018 16:33

updated README.md as per @nrc' comment

0fb92c6

collapsed exit-code handling

4860852

Removed write_mode from Summary, introduced partial option parsing ea…

a3006b9

…rlier

nrc mentioned this pull request Apr 10, 2018

Tracking issue: CI support #2614

Closed

2 tasks

Handle write-mode parsing in a slightly better way.

257715a

Merge branch 'master' of https://github.com/rust-lang-nursery/rustfmt

8755d1f

# Conflicts: # Cargo.lock # src/bin/main.rs

nrc merged commit f9532ba into rust-lang:master Apr 19, 2018

dzamlo mentioned this pull request May 6, 2018

Update README.md to reflect change in #2539 #2683

Merged

nrc added a commit that referenced this pull request May 6, 2018

Merge pull request #2683 from dzamlo/master

195489d

Update README.md to reflect change in #2539

bradjc added a commit to tock/tock that referenced this pull request May 18, 2018

rustfmt: diff is now check

3144f32

Per rust-lang/rustfmt#2539 I think check and diff do the same thing, except that check now returns a nonzero exit code if there are changes that rustfmt would make.

Implemented rough draft of check write mode. #2539

Implemented rough draft of check write mode. #2539

Conversation

davidbarsky commented Mar 17, 2018 • edited Loading

davidbarsky commented Mar 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Mar 18, 2018

davidbarsky commented Mar 18, 2018

davidbarsky commented Mar 19, 2018 • edited Loading

topecongiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbarsky Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

davidbarsky commented Mar 20, 2018

nrc commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbarsky Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

davidbarsky commented Mar 28, 2018

nrc commented Mar 28, 2018

davidbarsky commented Mar 31, 2018

nrc commented Apr 9, 2018

davidbarsky commented Apr 10, 2018

nrc commented Apr 10, 2018

davidbarsky commented Apr 19, 2018

davidbarsky commented Apr 19, 2018

nrc commented Apr 19, 2018

Implemented rough draft of `check` write mode. #2539

Implemented rough draft of `check` write mode. #2539

davidbarsky commented Mar 17, 2018 •

edited

Loading

davidbarsky commented Mar 19, 2018 •

edited

Loading

davidbarsky Mar 19, 2018 •

edited

Loading

davidbarsky Mar 25, 2018 •

edited

Loading