Skip to content

Implemented rough draft of check write mode. #2539

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 11 commits into from
Apr 19, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 149 additions & 0 deletions Cargo.lock

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ rustc-ap-syntax = "67.0.0"

[dev-dependencies]
lazy_static = "1.0.0"
assert_cli = "0.5"

[target.'cfg(unix)'.dependencies]
libc = "0.2.11"
Expand Down
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,16 @@ read data from stdin. Alternatively, you can use `cargo fmt` to format all
binary and library targets of your crate.

You'll probably want to specify the write mode. Currently, there are modes for
`diff`, `replace`, `overwrite`, `display`, `coverage`, `checkstyle`, and `plain`.
`check`, `diff`, `replace`, `overwrite`, `display`, `coverage`, `checkstyle`, and `plain`.

* `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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point; will do.

* `checkstyle` Will output the lines that need to be corrected as a checkstyle XML file,
that can be used by tools like Jenkins.

Expand Down
8 changes: 7 additions & 1 deletion src/bin/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ fn make_opts() -> Options {
"",
"write-mode",
"How to write output (not usable when piping from stdin)",
"[replace|overwrite|display|plain|diff|coverage|checkstyle]",
"[replace|overwrite|display|plain|diff|check|coverage|checkstyle]",
);

opts
Expand Down Expand Up @@ -287,6 +287,10 @@ fn execute(opts: &Options) -> FmtResult<Summary> {
}

let mut error_summary = Summary::default();
if let Some(mode) = options.write_mode {
error_summary.add_write_mode(mode)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for file in files {
if !file.exists() {
eprintln!("Error: file `{}` does not exist", file.to_str().unwrap());
Expand Down Expand Up @@ -342,6 +346,8 @@ fn main() {
Ok(summary) => {
if summary.has_operational_errors() {
1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@davidbarsky davidbarsky Mar 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else if summary.has_diff && summary.write_mode == Some(WriteMode::Check) {
1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else if summary.has_parsing_errors() {
2
} else if summary.has_formatting_errors() {
Expand Down
3 changes: 3 additions & 0 deletions src/config/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed/resolved.

Check,
}

configuration_option_enum! { Color:
Expand Down
9 changes: 9 additions & 0 deletions src/config/summary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
use std::time::{Duration, Instant};
use std::default::Default;

use config::options::WriteMode;

#[must_use]
#[derive(Debug, Default, Clone, Copy)]
pub struct Summary {
Expand All @@ -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>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll default it to overwrite?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved, defaults to Overwrite.


// Keeps track of time spent in parsing and formatting steps.
timer: Timer,
}
Expand Down Expand Up @@ -89,6 +94,10 @@ impl Summary {
self.has_diff = true;
}

pub fn add_write_mode(&mut self, mode: WriteMode) {
self.write_mode = Some(mode)
}

pub fn has_no_errors(&self) -> bool {
!(self.has_operational_errors || self.has_parsing_errors || self.has_formatting_errors
|| self.has_diff)
Expand Down
13 changes: 13 additions & 0 deletions src/filemap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,19 @@ where
let diff = create_diff(filename, text, config)?;
output_checkstyle_file(out, filename, diff)?;
}
WriteMode::Check => {
let filename = filename_to_path();
if let Ok((ori, fmt)) = source_and_formatted_text(text, filename, config) {
let mismatch = make_diff(&ori, &fmt, 3);
let has_diff = !mismatch.is_empty();
print_diff(
mismatch,
|line_num| format!("Diff in {} at line {}:", filename.display(), line_num),
config.color(),
);
return Ok(has_diff);
}
}
}

// when we are not in diff mode, don't indicate differing files
Expand Down
14 changes: 14 additions & 0 deletions tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

extern crate assert_cli;
#[macro_use]
extern crate lazy_static;
#[macro_use]
Expand Down Expand Up @@ -861,3 +862,16 @@ fn configuration_snippet_tests() {
println!("Ran {} configurations tests.", blocks.len());
assert_eq!(failures, 0, "{} configurations tests failed", failures);
}

#[test]
fn verify_check_works() {
assert_cli::Assert::command(&[
"cargo",
"run",
"--bin=rustfmt",
"--",
"--write-mode=check",
"src/bin/main.rs",
]).succeeds()
.unwrap();
}