From 2b95d3832b8c21fdca0cd5e2ccdbc571c7eb97da Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Tue, 10 Oct 2023 10:50:35 -0500 Subject: [PATCH] Update fix summary message in `check --diff` to include unsafe fix hints (#7790) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requires #7769 Updates the CLI display for `ruff check --fix` to hint availability of unsafe fixes. ``` ❯ ruff check example.py --select F601,T201 --diff --no-cache No errors fixed (1 fix available with `--unsafe-fixes`). ``` ``` ❯ ruff check example.py --select F601,T201,W292 --diff --no-cache --- example.py +++ example.py @@ -1,2 +1,2 @@ x = {'a': 1, 'a': 1} -print(('foo')) +print(('foo')) \ No newline at end of file Would fix 1 error (1 additional fix available with `--unsafe-fixes`). ``` ``` ❯ ruff check example.py --select F601,T201,W292 --diff --no-cache --unsafe-fixes --- example.py +++ example.py @@ -1,2 +1,2 @@ -x = {'a': 1} -print(('foo')) +x = {'a': 1, 'a': 1} +print(('foo')) \ No newline at end of file Would fix 2 errors. ``` --- crates/ruff_cli/src/printer.rs | 143 ++++++++++++---------- crates/ruff_cli/tests/integration_test.rs | 59 ++++++++- 2 files changed, 136 insertions(+), 66 deletions(-) diff --git a/crates/ruff_cli/src/printer.rs b/crates/ruff_cli/src/printer.rs index d1d5c36a84175..09a632261d4df 100644 --- a/crates/ruff_cli/src/printer.rs +++ b/crates/ruff_cli/src/printer.rs @@ -102,12 +102,15 @@ impl Printer { fn write_summary_text(&self, writer: &mut dyn Write, diagnostics: &Diagnostics) -> Result<()> { if self.log_level >= LogLevel::Default { + let fixables = FixableStatistics::try_from(diagnostics, self.unsafe_fixes); + + let fixed = diagnostics + .fixed + .values() + .flat_map(std::collections::HashMap::values) + .sum::(); + if self.flags.intersects(Flags::SHOW_VIOLATIONS) { - let fixed = diagnostics - .fixed - .values() - .flat_map(std::collections::HashMap::values) - .sum::(); let remaining = diagnostics.messages.len(); let total = fixed + remaining; if fixed > 0 { @@ -121,21 +124,73 @@ impl Printer { writeln!(writer, "Found {remaining} error{s}.")?; } - if let Some(fixables) = FixableSummary::try_from(diagnostics, self.unsafe_fixes) { - writeln!(writer, "{fixables}")?; + if let Some(fixables) = fixables { + let fix_prefix = format!("[{}]", "*".cyan()); + + if self.unsafe_fixes.is_enabled() { + writeln!( + writer, + "{fix_prefix} {} fixable with the --fix option.", + fixables.applicable + )?; + } else { + if fixables.applicable > 0 && fixables.unapplicable > 0 { + let es = if fixables.unapplicable == 1 { "" } else { "es" }; + writeln!(writer, + "{fix_prefix} {} fixable with the `--fix` option ({} hidden fix{es} can be enabled with the `--unsafe-fixes` option).", + fixables.applicable, fixables.unapplicable + )?; + } else if fixables.applicable > 0 { + // Only applicable fixes + writeln!( + writer, + "{fix_prefix} {} fixable with the `--fix` option.", + fixables.applicable, + )?; + } else { + // Only unapplicable fixes + let es = if fixables.unapplicable == 1 { "" } else { "es" }; + writeln!(writer, + "{} hidden fix{es} can be enabled with the `--unsafe-fixes` option.", + fixables.unapplicable + )?; + } + } } } else { - let fixed = diagnostics - .fixed - .values() - .flat_map(std::collections::HashMap::values) - .sum::(); - if fixed > 0 { - let s = if fixed == 1 { "" } else { "s" }; - if self.fix_mode.is_apply() { - writeln!(writer, "Fixed {fixed} error{s}.")?; + // Check if there are unapplied fixes + let unapplied = { + if let Some(fixables) = fixables { + fixables.unapplicable } else { - writeln!(writer, "Would fix {fixed} error{s}.")?; + 0 + } + }; + + if unapplied > 0 { + let es = if unapplied == 1 { "" } else { "es" }; + if fixed > 0 { + let s = if fixed == 1 { "" } else { "s" }; + if self.fix_mode.is_apply() { + writeln!(writer, "Fixed {fixed} error{s} ({unapplied} additional fix{es} available with `--unsafe-fixes`).")?; + } else { + writeln!(writer, "Would fix {fixed} error{s} ({unapplied} additional fix{es} available with `--unsafe-fixes`).")?; + } + } else { + if self.fix_mode.is_apply() { + writeln!(writer, "No errors fixed ({unapplied} fix{es} available with `--unsafe-fixes`).")?; + } else { + writeln!(writer, "No errors would be fixed ({unapplied} fix{es} available with `--unsafe-fixes`).")?; + } + } + } else { + if fixed > 0 { + let s = if fixed == 1 { "" } else { "s" }; + if self.fix_mode.is_apply() { + writeln!(writer, "Fixed {fixed} error{s}.")?; + } else { + writeln!(writer, "Would fix {fixed} error{s}.")?; + } } } } @@ -170,7 +225,7 @@ impl Printer { } let context = EmitterContext::new(&diagnostics.notebook_indexes); - let fixables = FixableSummary::try_from(diagnostics, self.unsafe_fixes); + let fixables = FixableStatistics::try_from(diagnostics, self.unsafe_fixes); match self.format { SerializationFormat::Json => { @@ -354,7 +409,7 @@ impl Printer { ); } - let fixables = FixableSummary::try_from(diagnostics, self.unsafe_fixes); + let fixables = FixableStatistics::try_from(diagnostics, self.unsafe_fixes); if !diagnostics.messages.is_empty() { if self.log_level >= LogLevel::Default { @@ -388,13 +443,13 @@ fn num_digits(n: usize) -> usize { } /// Return `true` if the [`Printer`] should indicate that a rule is fixable. -fn show_fix_status(fix_mode: flags::FixMode, fixables: Option<&FixableSummary>) -> bool { +fn show_fix_status(fix_mode: flags::FixMode, fixables: Option<&FixableStatistics>) -> bool { // If we're in application mode, avoid indicating that a rule is fixable. // If the specific violation were truly fixable, it would've been fixed in // this pass! (We're occasionally unable to determine whether a specific // violation is fixable without trying to fix it, so if fix is not // enabled, we may inadvertently indicate that a rule is fixable.) - (!fix_mode.is_apply()) && fixables.is_some_and(FixableSummary::any_applicable_fixes) + (!fix_mode.is_apply()) && fixables.is_some_and(FixableStatistics::any_applicable_fixes) } fn print_fix_summary(writer: &mut dyn Write, fixed: &FxHashMap) -> Result<()> { @@ -438,15 +493,14 @@ fn print_fix_summary(writer: &mut dyn Write, fixed: &FxHashMap Ok(()) } -/// Summarizes [applicable][ruff_diagnostics::Applicability] fixes. +/// Statistics for [applicable][ruff_diagnostics::Applicability] fixes. #[derive(Debug)] -struct FixableSummary { +struct FixableStatistics { applicable: u32, unapplicable: u32, - unsafe_fixes: UnsafeFixes, } -impl FixableSummary { +impl FixableStatistics { fn try_from(diagnostics: &Diagnostics, unsafe_fixes: UnsafeFixes) -> Option { let mut applicable = 0; let mut unapplicable = 0; @@ -467,7 +521,6 @@ impl FixableSummary { Some(Self { applicable, unapplicable, - unsafe_fixes, }) } } @@ -476,41 +529,3 @@ impl FixableSummary { self.applicable > 0 } } - -impl Display for FixableSummary { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let fix_prefix = format!("[{}]", "*".cyan()); - - if self.unsafe_fixes.is_enabled() { - write!( - f, - "{fix_prefix} {} fixable with the --fix option.", - self.applicable - ) - } else { - if self.applicable > 0 && self.unapplicable > 0 { - let es = if self.unapplicable == 1 { "" } else { "es" }; - write!( - f, - "{fix_prefix} {} fixable with the `--fix` option ({} hidden fix{es} can be enabled with the `--unsafe-fixes` option).", - self.applicable, self.unapplicable - ) - } else if self.applicable > 0 { - // Only applicable fixes - write!( - f, - "{fix_prefix} {} fixable with the `--fix` option.", - self.applicable, - ) - } else { - // Only unapplicable fixes - let es = if self.unapplicable == 1 { "" } else { "es" }; - write!( - f, - "{} hidden fix{es} can be enabled with the `--unsafe-fixes` option.", - self.unapplicable - ) - } - } - } -} diff --git a/crates/ruff_cli/tests/integration_test.rs b/crates/ruff_cli/tests/integration_test.rs index 7937af211cab3..caffc6a97c856 100644 --- a/crates/ruff_cli/tests/integration_test.rs +++ b/crates/ruff_cli/tests/integration_test.rs @@ -1035,6 +1035,35 @@ fn fix_applies_unsafe_fixes_with_opt_in() { "###); } +#[test] +fn fix_only_unsafe_fixes_available() { + assert_cmd_snapshot!( + Command::new(get_cargo_bin(BIN_NAME)) + .args([ + "-", + "--output-format", + "text", + "--isolated", + "--no-cache", + "--select", + "F601", + "--fix", + ]) + .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + x = {'a': 1, 'a': 1} + print(('foo')) + + ----- stderr ----- + -:1:14: F601 Dictionary key literal `'a'` repeated + Found 1 error. + 1 hidden fix can be enabled with the `--unsafe-fixes` option. + "###); +} + #[test] fn fix_only_flag_applies_safe_fixes_by_default() { assert_cmd_snapshot!( @@ -1058,7 +1087,7 @@ fn fix_only_flag_applies_safe_fixes_by_default() { print('foo') ----- stderr ----- - Fixed 1 error. + Fixed 1 error (1 additional fix available with `--unsafe-fixes`). "###); } @@ -1116,7 +1145,7 @@ fn diff_shows_safe_fixes_by_default() { ----- stderr ----- - Would fix 1 error. + Would fix 1 error (1 additional fix available with `--unsafe-fixes`). "### ); } @@ -1153,3 +1182,29 @@ fn diff_shows_unsafe_fixes_with_opt_in() { "### ); } + +#[test] +fn diff_only_unsafe_fixes_available() { + assert_cmd_snapshot!( + Command::new(get_cargo_bin(BIN_NAME)) + .args([ + "-", + "--output-format", + "text", + "--isolated", + "--no-cache", + "--select", + "F601", + "--diff", + ]) + .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + + ----- stderr ----- + No errors would be fixed (1 fix available with `--unsafe-fixes`). + "### + ); +}