Skip to content
11 changes: 11 additions & 0 deletions cli/args/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ pub struct EvalFlags {
#[derive(Clone, Default, Debug, Eq, PartialEq)]
pub struct FmtFlags {
pub check: bool,
pub fail_fast: bool,
pub files: FileFlags,
pub permit_no_files: bool,
pub use_tabs: Option<bool>,
Expand Down Expand Up @@ -2924,6 +2925,15 @@ Ignore formatting a file by adding an ignore comment at the top of the file:
.num_args(0)
.help_heading(FMT_HEADING),
)
.arg(
Arg::new("fail-fast")
.long("fail-fast")
.alias("failfast")
.help("Stop checking files on first format error")
.num_args(0)
.requires("check")
.help_heading(FMT_HEADING),
)
.arg(
Arg::new("ext")
.long("ext")
Expand Down Expand Up @@ -5826,6 +5836,7 @@ fn fmt_parse(

flags.subcommand = DenoSubcommand::Fmt(FmtFlags {
check: matches.get_flag("check"),
fail_fast: matches.get_flag("fail-fast"),
files: FileFlags { include, ignore },
permit_no_files: permit_no_files_parse(matches),
use_tabs,
Expand Down
25 changes: 23 additions & 2 deletions cli/tools/fmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ async fn format_files(
paths_with_options_batches: Vec<PathsWithOptions>,
) -> Result<(), AnyError> {
let formatter: Box<dyn Formatter> = if fmt_flags.check {
Box::new(CheckFormatter::default())
Box::new(CheckFormatter::new(fmt_flags.fail_fast))
Copy link
Member

Choose a reason for hiding this comment

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

It would be good for this to also be fail_fast when cli_options.is_quiet().

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @dsherret — updated this as well.
fail_fast is now enabled when either --fail-fast is passed or cli_options.is_quiet() is true.
Let me know if you'd prefer any tweaks!

} else {
Box::new(RealFormatter::default())
};
Expand Down Expand Up @@ -939,10 +939,22 @@ trait Formatter {
fn finish(&self) -> Result<(), AnyError>;
}

#[derive(Default)]
struct CheckFormatter {
not_formatted_files_count: Arc<AtomicUsize>,
checked_files_count: Arc<AtomicUsize>,
fail_fast: bool,
found_error: Arc<std::sync::atomic::AtomicBool>,
Copy link
Member

Choose a reason for hiding this comment

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

It seems this found_error is only relevant when fail_fast is true, so maybe do...

fail_fast_found_error: Option<Arc<std::sync::atomic::AtomicBool>>,

...to communicate that and to only do the work of setting it when fail_fast is true.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @dsherret — updated as suggested.
found_error is now replaced with
fail_fast_found_error: Option<Arc>,
and the atomic is only created/used when fail_fast is true.
Please let me know if you'd like any further adjustments!

}

impl CheckFormatter {
fn new(fail_fast: bool) -> Self {
Self {
not_formatted_files_count: Arc::new(AtomicUsize::new(0)),
checked_files_count: Arc::new(AtomicUsize::new(0)),
fail_fast,
found_error: Arc::new(std::sync::atomic::AtomicBool::new(false)),
}
}
}

#[async_trait]
Expand All @@ -961,7 +973,14 @@ impl Formatter for CheckFormatter {
run_parallelized(paths, {
let not_formatted_files_count = self.not_formatted_files_count.clone();
let checked_files_count = self.checked_files_count.clone();
let fail_fast = self.fail_fast;
let found_error = self.found_error.clone();
move |file_path| {
// Early exit if fail-fast is enabled and we've already found an error
if fail_fast && found_error.load(Ordering::Relaxed) {
return Ok(());
}

checked_files_count.fetch_add(1, Ordering::Relaxed);
let file = read_file_contents(&file_path)?;

Expand All @@ -981,6 +1000,7 @@ impl Formatter for CheckFormatter {
) {
Ok(Some(formatted_text)) => {
not_formatted_files_count.fetch_add(1, Ordering::Relaxed);
found_error.store(true, Ordering::Relaxed);
let _g = output_lock.lock();
let diff =
deno_resolver::display::diff(&file.text, &formatted_text);
Expand All @@ -1001,6 +1021,7 @@ impl Formatter for CheckFormatter {
}
Err(e) => {
not_formatted_files_count.fetch_add(1, Ordering::Relaxed);
found_error.store(true, Ordering::Relaxed);
let _g = output_lock.lock();
warn!("Error checking: {}", file_path.to_string_lossy());
warn!(
Expand Down
108 changes: 108 additions & 0 deletions tests/integration/fmt_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -439,3 +439,111 @@ fn test_tty_non_workspace_directory() {
pty.expect("Did not format non-workspace directory");
});
}

#[test]
fn fmt_check_fail_fast_stops_on_first_error() {
let context = TestContext::default();
let t = context.deno_dir();

// Create multiple badly formatted files
let bad1 = t.path().join("bad1.ts");
let bad2 = t.path().join("bad2.ts");
let bad3 = t.path().join("bad3.ts");

bad1.write("const a = 1\n");
bad2.write("const b = 2\n");
bad3.write("const c = 3\n");

// Run with --fail-fast
let output = context
.new_command()
.args_vec(vec![
"fmt".to_string(),
"--check".to_string(),
"--fail-fast".to_string(),
bad1.to_string(),
bad2.to_string(),
bad3.to_string(),
])
.run();

output.assert_exit_code(1);

// Should only report one file (the first one checked)
let output_text = output.combined_output();
assert_contains!(output_text, "Found 1 not formatted file");
}

#[test]
fn fmt_check_fail_fast_with_no_errors() {
let context = TestContext::default();
let t = context.deno_dir();

// Create properly formatted files
let good1 = t.path().join("good1.ts");
let good2 = t.path().join("good2.ts");

good1.write("const a = 1;\n");
good2.write("const b = 2;\n");

let output = context
.new_command()
.args_vec(vec![
"fmt".to_string(),
"--check".to_string(),
"--fail-fast".to_string(),
good1.to_string(),
good2.to_string(),
])
.run();

output.assert_exit_code(0);
assert_contains!(output.combined_output(), "Checked 2 files");
}

#[test]
fn fmt_check_fail_fast_requires_check() {
let context = TestContext::default();
let t = context.deno_dir();
let file = t.path().join("file.ts");
file.write("const a = 1;\n");

let output = context
.new_command()
.args_vec(vec![
"fmt".to_string(),
"--fail-fast".to_string(),
file.to_string(),
])
.run();

output.assert_exit_code(1);
assert_contains!(
output.combined_output(),
"the following required arguments were not provided"
);
}

#[test]
fn fmt_check_fail_fast_quiet() {
let context = TestContext::default();
let t = context.deno_dir();

let bad1 = t.path().join("bad1.ts");
bad1.write("const a = 1\n");

let output = context
.new_command()
.args_vec(vec![
"fmt".to_string(),
"--check".to_string(),
"--fail-fast".to_string(),
"--quiet".to_string(),
bad1.to_string(),
])
.run();

output.assert_exit_code(1);
// With --quiet, should have minimal output
assert_eq!(output.combined_output(), "");
}
Loading