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
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` Checks if a diff can be generated. If so, rustfmt outputs a diff and quits with exit code 1.
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 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."

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 like that; can do.

Copy link
Contributor Author

@davidbarsky davidbarsky Mar 25, 2018

Choose a reason for hiding this comment

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

Resolved.

This option is designed to be run in CI-like where a non-zero exit signifies non-standard code formatting.
* `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
15 changes: 9 additions & 6 deletions 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,13 +346,12 @@ 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 == 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
1
} else if summary.has_formatting_errors() {
3
} else if summary.has_diff {
// should only happen in diff mode
4
1
Copy link
Member

Choose a reason for hiding this comment

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

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

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 update later today.

} else {
assert!(summary.has_no_errors());
0
Expand Down
9 changes: 9 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,
// Checks if a diff can be generated. If so, rustfmt outputs a diff and quits with exit code 1.
// This option is designed to be run in CI where a non-zero exit signifies non-standard code formatting.
Check,
}

configuration_option_enum! { Color:
Expand Down Expand Up @@ -250,6 +253,12 @@ impl ::std::str::FromStr for WidthHeuristics {
}
}

impl Default for WriteMode {
fn default() -> WriteMode {
WriteMode::Overwrite
}
}

/// A set of directories, files and modules that rustfmt should ignore.
#[derive(Default, Deserialize, Serialize, Clone, Debug)]
pub struct IgnoreList(HashSet<PathBuf>);
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. Defaults to WriteMode::Overwrite.
pub write_mode: WriteMode,

// 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 = 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
27 changes: 27 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,29 @@ 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();
}

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