From 7208ec5b67bd590dbc35130a67986de3cced6bb1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Nov 2025 13:50:37 +0000 Subject: [PATCH] Add --add-noqa-reason flag to append custom comments to noqa directives Implements the ability to add custom reason/comment text when using the --add-noqa command. This allows users to distinguish auto-generated noqa comments from manual ones and add contextual information. Usage: ruff check --add-noqa --add-noqa-reason "TODO: fix later" The reason is appended after the rule codes in the noqa comment: # noqa: F401, F841 TODO: fix later Changes: - Add --add-noqa-reason CLI flag that requires --add-noqa - Thread reason parameter through add_noqa command chain - Update NoqaEdit struct to include optional reason field - Modify write() method to append reason before line ending - Update all test calls to pass None for backward compatibility Fixes #10070 Sanitize newlines in --add-noqa-reason to prevent syntax errors If a user passes a reason containing newline characters (e.g., via bash $'line1\nline2'), it would break Python syntax by creating actual line breaks in the noqa comment: import os # noqa: F401 line1 line2 # <- This becomes code, not a comment! This commit sanitizes the reason text by replacing all \n and \r characters with spaces, ensuring the noqa comment stays on one line. Added comprehensive test coverage for: - Unix newlines (\n) - Windows newlines (\r\n) - Multiple consecutive newlines Validate --add-noqa-reason for newlines instead of sanitizing Changed approach from silently sanitizing newlines to explicitly rejecting them with a clear error message. This follows the "fail fast" principle - it's better to alert users to problematic input rather than silently modifying it. The CLI now validates the reason text upfront and aborts with: "error: --add-noqa-reason cannot contain newline characters" This prevents syntax errors from malformed Python comments while being explicit about what input is accepted. Changes: - Add validation in lib.rs that checks for \n and \r characters - Remove sanitization code from noqa.rs write() method - Update test to verify normal reason text works correctly - Document that validation happens at CLI level Refactor --add-noqa to accept optional reason value directly Changed from two separate flags (--add-noqa and --add-noqa-reason) to a single flag with an optional value for better ergonomics. Before: ruff check --add-noqa --add-noqa-reason "TODO: fix" After: ruff check --add-noqa "TODO: fix" ruff check --add-noqa # still works without reason Implementation: - Changed `add_noqa: bool` to `add_noqa: Option` in args - Use clap's default_missing_value to handle flag without value - Empty string means flag present without value (no reason) - Non-empty string is the reason text to append - Removed separate add_noqa_reason field entirely This provides a cleaner, more intuitive CLI interface while maintaining full backward compatibility with the original --add-noqa behavior. Add comprehensive CLI tests for --add-noqa feature Created new test file tests/cli/add_noqa.rs with end-to-end tests: - add_noqa_without_reason: Verify original behavior still works - add_noqa_with_reason: Test appending custom reason text - add_noqa_with_short_reason: Test with short marker like "auto" - add_noqa_no_violations: Verify no changes when no violations - add_noqa_with_existing_comment: Test merging with existing noqa - add_noqa_multiple_violations_same_line: Test multiple codes - add_noqa_with_newline_error: Verify newline validation - add_noqa_multiple_files: Test bulk operations across files These tests verify: 1. Files are correctly modified in-place 2. Reason text is properly appended after rule codes 3. Multiple violations on same line are combined 4. Newlines are rejected with clear error message 5. Works correctly with multiple files Tests use insta snapshots for both command output and file contents. Simplify tests to only essential cases for --add-noqa reason Removed excessive integration tests and unit tests. Added only 2 focused tests to existing lint.rs test suite: 1. add_noqa_with_reason: Tests appending reason text to noqa directives 2. add_noqa_with_newline_in_reason: Validates newline rejection Also removed unit test from noqa.rs since integration tests are sufficient. Since --add-noqa is already well-tested with 7 existing tests covering base functionality, these 2 new tests are enough to verify the reason parameter feature. Fix test formatting to match existing test patterns - Remove leading whitespace from test input strings - Change snapshot format from @r###" to @r" to match existing tests - Input strings should not have indentation since dedent is applied This should fix CI test failures. Refactor tests to match existing --add-noqa test patterns Changes to follow the exact pattern of existing tests: - Use CliTest::new() + write_file() instead of with_file() - Use .check_command() which includes standard flags - Use separate .arg() calls instead of .args() - Pass specific filename instead of current directory This should fix remaining CI test failures. Fix CLI argument handling: require equals syntax for optional value Changes: - Add require_equals = true to --add-noqa argument definition - Update tests to use --add-noqa=value syntax instead of --add-noqa value - This makes the optional value syntax unambiguous Usage: ruff check --add-noqa # no reason ruff check --add-noqa="TODO: fix" # with reason This should fix CI test, clippy, and formatting failures. Simplify code to use .then() instead of if/else for clippy Fix formatting: use then_some instead of then for better idiomaticity Run cargo fmt to fix formatting Fix test snapshot indentation Remove leading spaces from inline snapshot expectations in add_noqa tests. The actual file content doesn't have leading indentation, so the snapshots shouldn't either. Fix add_noqa integration tests - Update add_noqa_with_reason test to trigger both F401 and F841 violations by using a function (F841 only applies to local variables, not module-level) - Fix error message snapshot for newline validation to match actual output format Fix clippy warning for inline format arg Nits, clippy, generate all --- crates/ruff/src/args.rs | 9 ++++- crates/ruff/src/commands/add_noqa.rs | 10 ++++- crates/ruff/src/lib.rs | 12 +++++- crates/ruff/tests/cli/lint.rs | 58 ++++++++++++++++++++++++++++ crates/ruff_linter/src/linter.rs | 2 + crates/ruff_linter/src/noqa.rs | 23 ++++++++++- docs/configuration.md | 5 ++- 7 files changed, 110 insertions(+), 9 deletions(-) diff --git a/crates/ruff/src/args.rs b/crates/ruff/src/args.rs index eb4bcd0a926e7..39dc0324fca51 100644 --- a/crates/ruff/src/args.rs +++ b/crates/ruff/src/args.rs @@ -405,8 +405,13 @@ pub struct CheckCommand { )] pub statistics: bool, /// Enable automatic additions of `noqa` directives to failing lines. + /// Optionally provide a reason to append after the codes. #[arg( long, + value_name = "REASON", + default_missing_value = "", + num_args = 0..=1, + require_equals = true, // conflicts_with = "add_noqa", conflicts_with = "show_files", conflicts_with = "show_settings", @@ -418,7 +423,7 @@ pub struct CheckCommand { conflicts_with = "fix", conflicts_with = "diff", )] - pub add_noqa: bool, + pub add_noqa: Option, /// See the files Ruff will be run against with the current settings. #[arg( long, @@ -1047,7 +1052,7 @@ Possible choices: /// etc.). #[expect(clippy::struct_excessive_bools)] pub struct CheckArguments { - pub add_noqa: bool, + pub add_noqa: Option, pub diff: bool, pub exit_non_zero_on_fix: bool, pub exit_zero: bool, diff --git a/crates/ruff/src/commands/add_noqa.rs b/crates/ruff/src/commands/add_noqa.rs index d5eaeb0170ccb..ff6a07c758a8e 100644 --- a/crates/ruff/src/commands/add_noqa.rs +++ b/crates/ruff/src/commands/add_noqa.rs @@ -21,6 +21,7 @@ pub(crate) fn add_noqa( files: &[PathBuf], pyproject_config: &PyprojectConfig, config_arguments: &ConfigArguments, + reason: Option<&str>, ) -> Result { // Collect all the files to check. let start = Instant::now(); @@ -76,7 +77,14 @@ pub(crate) fn add_noqa( return None; } }; - match add_noqa_to_path(path, package, &source_kind, source_type, &settings.linter) { + match add_noqa_to_path( + path, + package, + &source_kind, + source_type, + &settings.linter, + reason, + ) { Ok(count) => Some(count), Err(e) => { error!("Failed to add noqa to {}: {e}", path.display()); diff --git a/crates/ruff/src/lib.rs b/crates/ruff/src/lib.rs index 3bd457de8c942..3ea0d94fadcad 100644 --- a/crates/ruff/src/lib.rs +++ b/crates/ruff/src/lib.rs @@ -319,12 +319,20 @@ pub fn check(args: CheckCommand, global_options: GlobalConfigArgs) -> Result cannot contain newline characters" + )); + } + + let reason_opt = (!reason.is_empty()).then_some(reason.as_str()); + let modifications = - commands::add_noqa::add_noqa(&files, &pyproject_config, &config_arguments)?; + commands::add_noqa::add_noqa(&files, &pyproject_config, &config_arguments, reason_opt)?; if modifications > 0 && config_arguments.log_level >= LogLevel::Default { let s = if modifications == 1 { "" } else { "s" }; #[expect(clippy::print_stderr)] diff --git a/crates/ruff/tests/cli/lint.rs b/crates/ruff/tests/cli/lint.rs index ebd202b052b90..25500ed346c4f 100644 --- a/crates/ruff/tests/cli/lint.rs +++ b/crates/ruff/tests/cli/lint.rs @@ -1760,6 +1760,64 @@ from foo import ( # noqa: F401 Ok(()) } +#[test] +fn add_noqa_with_reason() -> Result<()> { + let fixture = CliTest::new()?; + fixture.write_file( + "test.py", + r#"import os + +def foo(): + x = 1 +"#, + )?; + + assert_cmd_snapshot!(fixture + .check_command() + .arg("--add-noqa=TODO: fix") + .arg("--select=F401,F841") + .arg("test.py"), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Added 2 noqa directives. + "); + + let content = fs::read_to_string(fixture.root().join("test.py"))?; + insta::assert_snapshot!(content, @r" +import os # noqa: F401 TODO: fix + +def foo(): + x = 1 # noqa: F841 TODO: fix +"); + + Ok(()) +} + +#[test] +fn add_noqa_with_newline_in_reason() -> Result<()> { + let fixture = CliTest::new()?; + fixture.write_file("test.py", "import os\n")?; + + assert_cmd_snapshot!(fixture + .check_command() + .arg("--add-noqa=line1\nline2") + .arg("--select=F401") + .arg("test.py"), @r###" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + ruff failed + Cause: --add-noqa cannot contain newline characters + "###); + + Ok(()) +} + /// Infer `3.11` from `requires-python` in `pyproject.toml`. #[test] fn requires_python() -> Result<()> { diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 2e4f284bee9e7..3ec070dd26385 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -377,6 +377,7 @@ pub fn add_noqa_to_path( source_kind: &SourceKind, source_type: PySourceType, settings: &LinterSettings, + reason: Option<&str>, ) -> Result { // Parse once. let target_version = settings.resolve_target_version(path); @@ -425,6 +426,7 @@ pub fn add_noqa_to_path( &settings.external, &directives.noqa_line_for, stylist.line_ending(), + reason, ) } diff --git a/crates/ruff_linter/src/noqa.rs b/crates/ruff_linter/src/noqa.rs index 606ac5ad3b526..da9535817ed07 100644 --- a/crates/ruff_linter/src/noqa.rs +++ b/crates/ruff_linter/src/noqa.rs @@ -39,7 +39,7 @@ pub fn generate_noqa_edits( let exemption = FileExemption::from(&file_directives); let directives = NoqaDirectives::from_commented_ranges(comment_ranges, external, path, locator); let comments = find_noqa_comments(diagnostics, locator, &exemption, &directives, noqa_line_for); - build_noqa_edits_by_diagnostic(comments, locator, line_ending) + build_noqa_edits_by_diagnostic(comments, locator, line_ending, None) } /// A directive to ignore a set of rules either for a given line of Python source code or an entire file (e.g., @@ -715,6 +715,7 @@ impl Display for LexicalError { impl Error for LexicalError {} /// Adds noqa comments to suppress all messages of a file. +#[expect(clippy::too_many_arguments)] pub(crate) fn add_noqa( path: &Path, diagnostics: &[Diagnostic], @@ -723,6 +724,7 @@ pub(crate) fn add_noqa( external: &[String], noqa_line_for: &NoqaMapping, line_ending: LineEnding, + reason: Option<&str>, ) -> Result { let (count, output) = add_noqa_inner( path, @@ -732,12 +734,14 @@ pub(crate) fn add_noqa( external, noqa_line_for, line_ending, + reason, ); fs::write(path, output)?; Ok(count) } +#[expect(clippy::too_many_arguments)] fn add_noqa_inner( path: &Path, diagnostics: &[Diagnostic], @@ -746,6 +750,7 @@ fn add_noqa_inner( external: &[String], noqa_line_for: &NoqaMapping, line_ending: LineEnding, + reason: Option<&str>, ) -> (usize, String) { let mut count = 0; @@ -757,7 +762,7 @@ fn add_noqa_inner( let comments = find_noqa_comments(diagnostics, locator, &exemption, &directives, noqa_line_for); - let edits = build_noqa_edits_by_line(comments, locator, line_ending); + let edits = build_noqa_edits_by_line(comments, locator, line_ending, reason); let contents = locator.contents(); @@ -783,6 +788,7 @@ fn build_noqa_edits_by_diagnostic( comments: Vec>, locator: &Locator, line_ending: LineEnding, + reason: Option<&str>, ) -> Vec> { let mut edits = Vec::default(); for comment in comments { @@ -794,6 +800,7 @@ fn build_noqa_edits_by_diagnostic( FxHashSet::from_iter([comment.code]), locator, line_ending, + reason, ) { edits.push(Some(noqa_edit.into_edit())); } @@ -808,6 +815,7 @@ fn build_noqa_edits_by_line<'a>( comments: Vec>>, locator: &Locator, line_ending: LineEnding, + reason: Option<&'a str>, ) -> BTreeMap> { let mut comments_by_line = BTreeMap::default(); for comment in comments.into_iter().flatten() { @@ -831,6 +839,7 @@ fn build_noqa_edits_by_line<'a>( .collect(), locator, line_ending, + reason, ) { edits.insert(offset, edit); } @@ -927,6 +936,7 @@ struct NoqaEdit<'a> { noqa_codes: FxHashSet<&'a SecondaryCode>, codes: Option<&'a Codes<'a>>, line_ending: LineEnding, + reason: Option<&'a str>, } impl NoqaEdit<'_> { @@ -954,6 +964,9 @@ impl NoqaEdit<'_> { push_codes(writer, self.noqa_codes.iter().sorted_unstable()); } } + if let Some(reason) = self.reason { + write!(writer, " {reason}").unwrap(); + } write!(writer, "{}", self.line_ending.as_str()).unwrap(); } } @@ -970,6 +983,7 @@ fn generate_noqa_edit<'a>( noqa_codes: FxHashSet<&'a SecondaryCode>, locator: &Locator, line_ending: LineEnding, + reason: Option<&'a str>, ) -> Option> { let line_range = locator.full_line_range(offset); @@ -999,6 +1013,7 @@ fn generate_noqa_edit<'a>( noqa_codes, codes, line_ending, + reason, }) } @@ -2832,6 +2847,7 @@ mod tests { &[], &noqa_line_for, LineEnding::Lf, + None, ); assert_eq!(count, 0); assert_eq!(output, format!("{contents}")); @@ -2855,6 +2871,7 @@ mod tests { &[], &noqa_line_for, LineEnding::Lf, + None, ); assert_eq!(count, 1); assert_eq!(output, "x = 1 # noqa: F841\n"); @@ -2885,6 +2902,7 @@ mod tests { &[], &noqa_line_for, LineEnding::Lf, + None, ); assert_eq!(count, 1); assert_eq!(output, "x = 1 # noqa: E741, F841\n"); @@ -2915,6 +2933,7 @@ mod tests { &[], &noqa_line_for, LineEnding::Lf, + None, ); assert_eq!(count, 0); assert_eq!(output, "x = 1 # noqa"); diff --git a/docs/configuration.md b/docs/configuration.md index 8d3297fbcab8e..7a5f62fc60530 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -618,8 +618,9 @@ Options: notebooks, use `--extension ipy:ipynb` --statistics Show counts for every rule with at least one violation - --add-noqa - Enable automatic additions of `noqa` directives to failing lines + --add-noqa[=] + Enable automatic additions of `noqa` directives to failing lines. + Optionally provide a reason to append after the codes --show-files See the files Ruff will be run against with the current settings --show-settings