feat(fmt): add --fail-fast flag to deno fmt --check #31438
Conversation
Adds a --fail-fast flag to deno fmt --check that stops checking files immediately after finding the first formatting error. - Uses AtomicBool for thread-safe early exit signaling - Workers check the flag before processing each file - Follows the same pattern as deno test --fail-fast - Includes 4 comprehensive test cases
WalkthroughAdds a Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/args/flags.rs (1)
8032-8359: Update FmtFlags struct literals in tests to includefail_fastAll the
FmtFlags { ... }literals in thefmttests (for example infn fmt()around constructingDenoSubcommand::Fmt(FmtFlags { ... })) do not initialize the newfail_fastfield. Withpub struct FmtFlags { pub check: bool, pub fail_fast: bool, ... }, these literals will no longer compile.Initialize
fail_fastexplicitly (likelyfalse) in each FmtFlags test literal, e.g.:- subcommand: DenoSubcommand::Fmt(FmtFlags { - check: false, + subcommand: DenoSubcommand::Fmt(FmtFlags { + check: false, + fail_fast: false, files: FileFlags { include: vec!["script_1.ts".to_string(), "script_2.ts".to_string()], ignore: vec![], }, permit_no_files: false, // ... }),Apply the same addition to the other FmtFlags literals in this test module so the tests build again.
🧹 Nitpick comments (1)
cli/tools/fmt.rs (1)
942-957: LGTM!Struct and constructor are well-designed. Minor style note:
AtomicBoolcould be imported at the top of the file alongsideAtomicUsize(line 19-20) to avoid the full pathstd::sync::atomic::AtomicBool.use std::sync::atomic::AtomicUsize; +use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering;Then simplify:
- found_error: Arc<std::sync::atomic::AtomicBool>, + found_error: Arc<AtomicBool>,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli/args/flags.rs(3 hunks)cli/tools/fmt.rs(5 hunks)tests/integration/fmt_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
cli/args/flags.rs
📄 CodeRabbit inference engine (CLAUDE.md)
CLI flag parsing should be defined in
cli/args/flags.rs
Files:
cli/args/flags.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
cli/args/flags.rscli/tools/fmt.rstests/integration/fmt_tests.rs
cli/tools/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
CLI tools should be implemented in
cli/tools/<tool>orcli/tools/<tool>/mod.rs
Files:
cli/tools/fmt.rs
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to cli/args/flags.rs : CLI flag parsing should be defined in `cli/args/flags.rs`
Applied to files:
cli/args/flags.rs
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to src/**/*.rs : Unit tests should be inline with source code in each module
Applied to files:
tests/integration/fmt_tests.rs
🧬 Code graph analysis (1)
tests/integration/fmt_tests.rs (1)
tests/util/server/src/builders.rs (1)
combined_output(1016-1022)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test release linux-aarch64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test release windows-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug linux-x86_64
- GitHub Check: lint debug macos-x86_64
🔇 Additional comments (8)
tests/integration/fmt_tests.rs (4)
443-475: Potential test flakiness due to parallel execution.The test asserts exactly "Found 1 not formatted file", but with parallel execution and
Ordering::Relaxedon thefound_erroratomic flag, multiple files could be processed before any thread observes the flag. Consider either:
- Asserting a range (e.g., at least 1 but fewer than 3 files)
- Using
assert_contains!(output_text, "not formatted")without the exact count- Adding the
#[flaky_test::flaky_test]attribute if some non-determinism is acceptable
477-502: LGTM!Clean test case for the happy path when all files are properly formatted.
504-525: LGTM!Good coverage for the CLI constraint enforcement.
527-549: LGTM!Solid test for verifying quiet mode properly suppresses output while still returning the correct exit code.
cli/tools/fmt.rs (3)
229-233: LGTM!Clean integration of the fail-fast flag propagation into the formatter selection.
976-984: LGTM!The early exit logic is correctly placed before incrementing
checked_files_count. UsingOrdering::Relaxedis acceptable here since exact synchronization isn't required—stopping "soon" after an error is sufficient for the fail-fast optimization.
1001-1042: LGTM!The
found_errorflag is correctly set in both the formatting difference case (line 1003) and the error case (line 1024), ensuring fail-fast triggers on any type of failure.cli/args/flags.rs (1)
263-276: Fmt--fail-fastflag wiring looks consistent and scoped correctly
- Adding
fail_fast: booltoFmtFlagsand parsing it viamatches.get_flag("fail-fast")is straightforward and keeps the model aligned with other fmt options.- The new
--fail-fast/--failfastCLI arg is correctly grouped underFMT_HEADINGand constrained with.requires("check"), so it only applies todeno fmt --checkflows.No issues from a flags/parsing perspective.
Also applies to: 2918-2936, 5810-5851
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/args/flags.rs (1)
8032-8370: Fmt flag tests updated appropriately; optional: add afail_fast = truecaseThe
fmt()tests were updated to includefail_fast: falsein all existing expectations, which keeps the equality checks robust after extendingFmtFlags.If you want symmetry with how
test --fail-fastis covered, consider adding a smallflags_from_vec(["deno","fmt","--check","--fail-fast", ...])assertion that expectsfail_fast: true, but this is purely incremental and not blocking given the integration tests mentioned in the PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/args/flags.rs(14 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
cli/args/flags.rs
📄 CodeRabbit inference engine (CLAUDE.md)
CLI flag parsing should be defined in
cli/args/flags.rs
Files:
cli/args/flags.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
cli/args/flags.rs
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to cli/args/flags.rs : CLI flag parsing should be defined in `cli/args/flags.rs`
Applied to files:
cli/args/flags.rs
🧬 Code graph analysis (1)
cli/args/flags.rs (1)
cli/tools/fmt.rs (1)
new(950-957)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: build libs
🔇 Additional comments (1)
cli/args/flags.rs (1)
263-278: Fail‑fast flag wiring fordeno fmtlooks correct and consistent
- Adding
fail_fast: booltoFmtFlagsand threading it viafmt_subcommand()’s--fail-fast/--failfastflag intofmt_parseis coherent and mirrors how other fmt options are handled..requires("check")correctly enforces that fail‑fast is only usable with--check, matching the stated behavior without impacting watch/config resolution.No changes needed here from my side.
Also applies to: 2918-2936, 5817-5851
|
Has this feature been requested by anyone? Seems very specific and niche usecase. |
|
Hi @bartlomieju, yes—this feature is helpful for users running large codebases in
Even if the use case is smaller, similar behaviour exists in other |
- Adds --fail-fast flag that stops on first formatting error - Uses AtomicBool for thread-safe early exit - Includes 4 comprehensive test cases - Fixes test expectation for --quiet mode
- Adds --fail-fast flag that stops on first formatting error - Uses AtomicBool for thread-safe early exit in parallel processing - Includes 4 comprehensive test cases - Fixes: Added fail_fast field to all FmtFlags test initializers - Fixes: Updated quiet mode test to match actual Deno behavior - Fixes: Made fail-fast test robust to parallel processing race conditions
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/integration/fmt_tests.rs (2)
443-479: Test does not actually assert “stops on first error” semanticsRight now this would still pass even if
--fail-fastwere a no-op, since it only checks for exit code1and that some “not formatted file” text appears—both true for plainfmt --checkwith bad files. If you want this to guard the new behavior, consider tightening it (for example by asserting on the reported checked count vs a baseline without--fail-fast, or otherwise verifying we don’t report all files), or relax the test name/comment so it doesn’t promise behavior we don’t assert.
531-554: Quiet-mode interaction is mostly covered; optionally assert info suppressionThis test ensures that
--quietstill surfaces the formatting error summary, which is the critical behavior. If you want stronger coverage of the quiet semantics, you could also assert that informational lines like “Checked N files” are absent:- let output_text = output.combined_output(); - assert_contains!(output_text, "Found 1 not formatted file"); + let output_text = output.combined_output(); + assert_contains!(output_text, "Found 1 not formatted file"); + // Ensure quiet mode suppresses non-error info messages. + assert_not_contains!(output_text, "Checked 1 file");Purely optional, but would make regressions in quiet handling more visible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/fmt_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
tests/integration/fmt_tests.rs
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to src/**/*.rs : Unit tests should be inline with source code in each module
Applied to files:
tests/integration/fmt_tests.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug linux-x86_64
🔇 Additional comments (2)
tests/integration/fmt_tests.rs (2)
481-506: Happy path coverage for fail-fast with clean files looks goodThis test validates that
--fail-fastdoesn’t change behavior when everything is already formatted and that the checked-files summary is still emitted. The setup and assertions look solid and consistent with surrounding tests.
508-529: Argument-validation behavior for--fail-fastwithout--checkis well coveredThis cleanly exercises the “requires
--check” constraint and asserts both non-zero exit and a generic “required arguments” message, which should be reasonably stable across CLI changes.
|
Hi @bartlomieju, @Tango992, can you please review. |
cli/tools/fmt.rs
Outdated
| fail_fast: bool, | ||
| found_error: Arc<std::sync::atomic::AtomicBool>, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
cli/tools/fmt.rs
Outdated
| ) -> Result<(), AnyError> { | ||
| let formatter: Box<dyn Formatter> = if fmt_flags.check { | ||
| Box::new(CheckFormatter::default()) | ||
| Box::new(CheckFormatter::new(fmt_flags.fail_fast)) |
There was a problem hiding this comment.
It would be good for this to also be fail_fast when cli_options.is_quiet().
There was a problem hiding this comment.
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!
- Convert to optional atomic pattern (only allocate when needed) - Auto-enable fail-fast when --quiet flag is used - Update constructor to conditionally create Arc<AtomicBool> - Use if-let pattern for cleaner early-exit checks
Fixes clippy::collapsible_if warning by combining if-let and if conditions
|
Kindly review when you get time. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/args/flags.rs (1)
8134-8472: Fmt tests updated forfail_fastbut lack a positive CLI parse caseAll existing
fmttests were correctly adjusted to expectfail_fast: false, so equality assertions still compile and validate defaults. To lock in the new flag’s parsing, consider adding a small unit test that asserts--checkplus--fail-fastyieldsfail_fast: true, similar in spirit totest_with_fail_fastfor the test subcommand:#[test] fn fmt_with_fail_fast() { let r = flags_from_vec(svec!["deno", "fmt", "--check", "--fail-fast"]).unwrap(); let Flags { subcommand, .. } = r; if let DenoSubcommand::Fmt(FmtFlags { check, fail_fast, .. }) = subcommand { assert!(check); assert!(fail_fast); } else { panic!("expected Fmt subcommand"); } }This would catch any future regressions in the CLI wiring for the new flag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/args/flags.rs(14 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
cli/args/flags.rs
📄 CodeRabbit inference engine (CLAUDE.md)
CLI flag parsing should be defined in
cli/args/flags.rs
Files:
cli/args/flags.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
cli/args/flags.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
cli/args/flags.rs
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to cli/args/flags.rs : CLI flag parsing should be defined in `cli/args/flags.rs`
Applied to files:
cli/args/flags.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: build libs
🔇 Additional comments (3)
cli/args/flags.rs (3)
260-275:FmtFlagsextension withfail_fastis consistent and safeAdding
pub fail_fast: boolnext tocheckkeeps related concerns together, preservesDefaultbehavior (false), and doesn’t affect existing pattern matches (FmtFlags { files, .. },watch) elsewhere in this file. No issues spotted.
2991-3009:fmt --fail-fastflag definition is well-scopedThe new
--fail-fast/--failfastoption is correctly added under the formatting heading, and tying it to--checkvia.requires("check")matches the intended “check-mode only” semantics and mirrors the test subcommand’s flag naming.
5899-5942: Plumbingfail_fastintoFmtFlags/DenoSubcommand::Fmtlooks correct
fmt_parsenow threadsmatches.get_flag("fail-fast")intoFmtFlags.fail_fastalongsidecheck, so downstream tools see the flag without any extra branching. This is in line with how other boolean CLI switches are handled here.
3 participants
issue no :
#26585
Adds a
--fail-fastflag todeno fmt --checkthat stops checking files immediately after finding the first formatting error, matching the behavior ofdeno test --fail-fast.When running
deno fmt --checkon large codebases, developers often want immediate feedback on the first formatting issue rather than waiting for all files to be checked. This flag enables:deno test --fail-fastbehavior