Skip to content

Commit

Permalink
Update fix summary message in check --diff to include unsafe fix hi…
Browse files Browse the repository at this point in the history 
…nts (#7790)

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.
```
  • Loading branch information
@zanieb
zanieb committed Oct 10, 2023
1 parent d412e8e commit 2b95d38
Show file tree
Hide file tree
Showing 2 changed files with 136 additions and 66 deletions.
143 changes: 79 additions & 64 deletions crates/ruff_cli/src/printer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::<usize>();

if self.flags.intersects(Flags::SHOW_VIOLATIONS) {
let fixed = diagnostics
.fixed
.values()
.flat_map(std::collections::HashMap::values)
.sum::<usize>();
let remaining = diagnostics.messages.len();
let total = fixed + remaining;
if fixed > 0 {
Expand All @@ -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::<usize>();
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}.")?;
}
}
}
}
Expand Down Expand Up @@ -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 => {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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<String, FixTable>) -> Result<()> {
Expand Down Expand Up @@ -438,15 +493,14 @@ fn print_fix_summary(writer: &mut dyn Write, fixed: &FxHashMap<String, FixTable>
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<Self> {
let mut applicable = 0;
let mut unapplicable = 0;
Expand All @@ -467,7 +521,6 @@ impl FixableSummary {
Some(Self {
applicable,
unapplicable,
unsafe_fixes,
})
}
}
Expand All @@ -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
)
}
}
}
}
59 changes: 57 additions & 2 deletions crates/ruff_cli/tests/integration_test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand All @@ -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`).
"###);
}

Expand Down Expand Up @@ -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`).
"###
);
}
Expand Down Expand Up @@ -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`).
"###
);
}

0 comments on commit 2b95d38

Please sign in to comment.