Skip to content

Commit f272eb8

Browse files
ntBresecond-ed
authored andcommitted
Show fixes by default (astral-sh#19919)
## Summary This PR fixes astral-sh#7352 by exposing the `show_fix_diff` option used in our snapshot tests in the CLI. As the issue suggests, we plan to make this the default output format in the future, so this is added to the `full` output format in preview for now. This turned out to be pretty straightforward. I just used our existing `Applicability` settings to determine whether or not to print the diff. The snapshot differences are because we now set `Applicability::DisplayOnly` for our snapshot tests. This `Applicability` is also used to determine whether or not the fix icon (`[*]`) is rendered, so this is now shown for display-only fixes in our snapshots. This was already the case previously, but we were only setting `Applicability::Unsafe` in these tests and ignoring the `Applicability` when rendering fix diffs. CLI users can't enable display-only fixes, so this is only a test change for now, but this should work smoothly if we decide to expose a `--display-only-fixes` flag or similar in the future. I also deleted the `PrinterFlags::SHOW_FIX_DIFF` flag. This was completely unused before, and it seemed less confusing just to delete it than to enable it in the right place and check it along with the `OutputFormat` and `preview`. ## Test Plan I only added one CLI test for now. I'm kind of assuming that we have decent coverage of the cases where this shouldn't be firing, especially the `output_format` CLI test, which shows that this definitely doesn't affect non-preview `full` output. I'm happy to add more tests with different combinations of options, if we're worried about any in particular. I did try `--diff` and `--preview` and a few other combinations manually. And here's a screenshot using our trusty UP049 example from the design discussion confirming that all the colors and other formatting still look as expected: <img width="786" height="629" alt="image" src="https://github.com/user-attachments/assets/94e408bc-af7b-4573-b546-a5ceac2620f2" /> And one with an unsafe fix to see the footer: <img width="782" height="367" alt="image" src="https://github.com/user-attachments/assets/bbb29e47-310b-4293-b2c2-cc7aee3baff4" /> ## Related issues and PR - astral-sh#7352 - astral-sh#12595 - astral-sh#12598 - astral-sh#12599 - astral-sh#12600 I think we could probably close all of these issues now. I think we've either resolved or avoided most of them, and if we encounter them again with the new output format, it would probably make sense to open new ones anyway.
1 parent 08f3f22 commit f272eb8

18 files changed

+133
-100
lines changed

crates/ruff/src/printer.rs

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,6 @@ bitflags! {
3030
const SHOW_VIOLATIONS = 1 << 0;
3131
/// Whether to show a summary of the fixed violations when emitting diagnostics.
3232
const SHOW_FIX_SUMMARY = 1 << 1;
33-
/// Whether to show a diff of each fixed violation when emitting diagnostics.
34-
const SHOW_FIX_DIFF = 1 << 2;
3533
}
3634
}
3735

@@ -260,9 +258,9 @@ impl Printer {
260258
OutputFormat::Concise | OutputFormat::Full => {
261259
TextEmitter::default()
262260
.with_show_fix_status(show_fix_status(self.fix_mode, fixables.as_ref()))
263-
.with_show_fix_diff(self.flags.intersects(Flags::SHOW_FIX_DIFF))
261+
.with_show_fix_diff(self.format == OutputFormat::Full && preview)
264262
.with_show_source(self.format == OutputFormat::Full)
265-
.with_unsafe_fixes(self.unsafe_fixes)
263+
.with_fix_applicability(self.unsafe_fixes.required_applicability())
266264
.with_preview(preview)
267265
.emit(writer, &diagnostics.inner, &context)?;
268266

@@ -464,7 +462,7 @@ impl Printer {
464462
TextEmitter::default()
465463
.with_show_fix_status(show_fix_status(self.fix_mode, fixables.as_ref()))
466464
.with_show_source(preview)
467-
.with_unsafe_fixes(self.unsafe_fixes)
465+
.with_fix_applicability(self.unsafe_fixes.required_applicability())
468466
.emit(writer, &diagnostics.inner, &context)?;
469467
}
470468
writer.flush()?;

crates/ruff/tests/lint.rs

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5830,3 +5830,33 @@ nested_optional: Optional[Optional[Optional[str]]] = None
58305830
",
58315831
);
58325832
}
5833+
5834+
#[test]
5835+
fn show_fixes_in_full_output_with_preview_enabled() {
5836+
assert_cmd_snapshot!(
5837+
Command::new(get_cargo_bin(BIN_NAME))
5838+
.args(["check", "--no-cache", "--output-format", "full"])
5839+
.args(["--select", "F401"])
5840+
.arg("--preview")
5841+
.arg("-")
5842+
.pass_stdin("import math"),
5843+
@r"
5844+
success: false
5845+
exit_code: 1
5846+
----- stdout -----
5847+
F401 [*] `math` imported but unused
5848+
--> -:1:8
5849+
|
5850+
1 | import math
5851+
| ^^^^
5852+
|
5853+
help: Remove unused import: `math`
5854+
- import math
5855+
5856+
Found 1 error.
5857+
[*] 1 fixable with the `--fix` option.
5858+
5859+
----- stderr -----
5860+
",
5861+
);
5862+
}

crates/ruff_db/src/diagnostic/mod.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,13 @@ impl Diagnostic {
349349
self.fix().is_some()
350350
}
351351

352+
/// Returns `true` if the diagnostic is [`fixable`](Diagnostic::fixable) and applies at the
353+
/// configured applicability level.
354+
pub fn has_applicable_fix(&self, config: &DisplayDiagnosticConfig) -> bool {
355+
self.fix()
356+
.is_some_and(|fix| fix.applies(config.fix_applicability))
357+
}
358+
352359
/// Returns the offset of the parent statement for this diagnostic if it exists.
353360
///
354361
/// This is primarily used for checking noqa/secondary code suppressions.

crates/ruff_db/src/diagnostic/render.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -254,9 +254,7 @@ impl<'a> ResolvedDiagnostic<'a> {
254254
id,
255255
message: diag.inner.message.as_str().to_string(),
256256
annotations,
257-
is_fixable: diag
258-
.fix()
259-
.is_some_and(|fix| fix.applies(config.fix_applicability)),
257+
is_fixable: diag.has_applicable_fix(config),
260258
}
261259
}
262260

crates/ruff_db/src/diagnostic/render/concise.rs

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,11 +77,9 @@ impl<'a> ConciseRenderer<'a> {
7777
)?;
7878
}
7979
if self.config.show_fix_status {
80-
if let Some(fix) = diag.fix() {
81-
// Do not display an indicator for inapplicable fixes
82-
if fix.applies(self.config.fix_applicability) {
83-
write!(f, "[{fix}] ", fix = fmt_styled("*", stylesheet.separator))?;
84-
}
80+
// Do not display an indicator for inapplicable fixes
81+
if diag.has_applicable_fix(self.config) {
82+
write!(f, "[{fix}] ", fix = fmt_styled("*", stylesheet.separator))?;
8583
}
8684
}
8785
} else {

crates/ruff_db/src/diagnostic/render/full.rs

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ impl<'a> FullRenderer<'a> {
5858
writeln!(f, "{}", renderer.render(diag.to_annotate()))?;
5959
}
6060

61-
if self.config.show_fix_diff {
61+
if self.config.show_fix_diff && diag.has_applicable_fix(self.config) {
6262
if let Some(diff) = Diff::from_diagnostic(diag, &stylesheet, self.resolver) {
6363
write!(f, "{diff}")?;
6464
}
@@ -697,6 +697,8 @@ print()
697697
fn notebook_output_with_diff() {
698698
let (mut env, diagnostics) = create_notebook_diagnostics(DiagnosticFormat::Full);
699699
env.show_fix_diff(true);
700+
env.fix_applicability(Applicability::DisplayOnly);
701+
700702
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
701703
error[unused-import][*]: `os` imported but unused
702704
--> notebook.ipynb:cell 1:2:8
@@ -726,7 +728,7 @@ print()
726728
2 |
727729
3 | print('hello world')
728730
729-
error[unused-variable]: Local variable `x` is assigned to but never used
731+
error[unused-variable][*]: Local variable `x` is assigned to but never used
730732
--> notebook.ipynb:cell 3:4:5
731733
|
732734
2 | def foo():
@@ -749,6 +751,7 @@ print()
749751
fn notebook_output_with_diff_spanning_cells() {
750752
let (mut env, mut diagnostics) = create_notebook_diagnostics(DiagnosticFormat::Full);
751753
env.show_fix_diff(true);
754+
env.fix_applicability(Applicability::DisplayOnly);
752755

753756
// Move all of the edits from the later diagnostics to the first diagnostic to simulate a
754757
// single diagnostic with edits in different cells.
@@ -761,7 +764,7 @@ print()
761764
*fix = Fix::unsafe_edits(edits.remove(0), edits);
762765

763766
insta::assert_snapshot!(env.render(&diagnostic), @r"
764-
error[unused-import]: `os` imported but unused
767+
error[unused-import][*]: `os` imported but unused
765768
--> notebook.ipynb:cell 1:2:8
766769
|
767770
1 | # cell 1
@@ -924,6 +927,7 @@ line 10
924927
env.add("example.py", contents);
925928
env.format(DiagnosticFormat::Full);
926929
env.show_fix_diff(true);
930+
env.fix_applicability(Applicability::DisplayOnly);
927931

928932
let mut diagnostic = env.err().primary("example.py", "3", "3", "label").build();
929933
diagnostic.help("Start of diff:");
@@ -936,7 +940,7 @@ line 10
936940
)));
937941

938942
insta::assert_snapshot!(env.render(&diagnostic), @r"
939-
error[test-diagnostic]: main diagnostic message
943+
error[test-diagnostic][*]: main diagnostic message
940944
--> example.py:3:1
941945
|
942946
1 | line 1

crates/ruff_linter/src/message/text.rs

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,9 @@ use std::io::Write;
33
use ruff_db::diagnostic::{
44
Diagnostic, DiagnosticFormat, DisplayDiagnosticConfig, DisplayDiagnostics,
55
};
6+
use ruff_diagnostics::Applicability;
67

78
use crate::message::{Emitter, EmitterContext};
8-
use crate::settings::types::UnsafeFixes;
99

1010
pub struct TextEmitter {
1111
config: DisplayDiagnosticConfig,
@@ -46,10 +46,8 @@ impl TextEmitter {
4646
}
4747

4848
#[must_use]
49-
pub fn with_unsafe_fixes(mut self, unsafe_fixes: UnsafeFixes) -> Self {
50-
self.config = self
51-
.config
52-
.fix_applicability(unsafe_fixes.required_applicability());
49+
pub fn with_fix_applicability(mut self, applicability: Applicability) -> Self {
50+
self.config = self.config.fix_applicability(applicability);
5351
self
5452
}
5553

@@ -86,13 +84,13 @@ impl Emitter for TextEmitter {
8684
#[cfg(test)]
8785
mod tests {
8886
use insta::assert_snapshot;
87+
use ruff_diagnostics::Applicability;
8988

9089
use crate::message::TextEmitter;
9190
use crate::message::tests::{
9291
capture_emitter_notebook_output, capture_emitter_output, create_diagnostics,
9392
create_notebook_diagnostics, create_syntax_error_diagnostics,
9493
};
95-
use crate::settings::types::UnsafeFixes;
9694

9795
#[test]
9896
fn default() {
@@ -117,7 +115,7 @@ mod tests {
117115
let mut emitter = TextEmitter::default()
118116
.with_show_fix_status(true)
119117
.with_show_source(true)
120-
.with_unsafe_fixes(UnsafeFixes::Enabled);
118+
.with_fix_applicability(Applicability::Unsafe);
121119
let content = capture_emitter_output(&mut emitter, &create_diagnostics());
122120

123121
assert_snapshot!(content);
@@ -128,7 +126,7 @@ mod tests {
128126
let mut emitter = TextEmitter::default()
129127
.with_show_fix_status(true)
130128
.with_show_source(true)
131-
.with_unsafe_fixes(UnsafeFixes::Enabled);
129+
.with_fix_applicability(Applicability::Unsafe);
132130
let (messages, notebook_indexes) = create_notebook_diagnostics();
133131
let content = capture_emitter_notebook_output(&mut emitter, &messages, &notebook_indexes);
134132

crates/ruff_linter/src/rules/eradicate/snapshots/ruff_linter__rules__eradicate__tests__ERA001_ERA001.py.snap

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
---
22
source: crates/ruff_linter/src/rules/eradicate/mod.rs
33
---
4-
ERA001 Found commented-out code
4+
ERA001 [*] Found commented-out code
55
--> ERA001.py:1:1
66
|
77
1 | #import os
@@ -16,7 +16,7 @@ help: Remove commented-out code
1616
3 | a = 4
1717
note: This is a display-only fix and is likely to be incorrect
1818

19-
ERA001 Found commented-out code
19+
ERA001 [*] Found commented-out code
2020
--> ERA001.py:2:1
2121
|
2222
1 | #import os
@@ -33,7 +33,7 @@ help: Remove commented-out code
3333
4 | #foo(1, 2, 3)
3434
note: This is a display-only fix and is likely to be incorrect
3535

36-
ERA001 Found commented-out code
36+
ERA001 [*] Found commented-out code
3737
--> ERA001.py:3:1
3838
|
3939
1 | #import os
@@ -52,7 +52,7 @@ help: Remove commented-out code
5252
5 |
5353
note: This is a display-only fix and is likely to be incorrect
5454

55-
ERA001 Found commented-out code
55+
ERA001 [*] Found commented-out code
5656
--> ERA001.py:5:1
5757
|
5858
3 | #a = 3
@@ -72,7 +72,7 @@ help: Remove commented-out code
7272
7 | content = 1 # print('hello')
7373
note: This is a display-only fix and is likely to be incorrect
7474

75-
ERA001 Found commented-out code
75+
ERA001 [*] Found commented-out code
7676
--> ERA001.py:13:5
7777
|
7878
11 | # This is a real comment.
@@ -91,7 +91,7 @@ help: Remove commented-out code
9191
15 | #import os # noqa: ERA001
9292
note: This is a display-only fix and is likely to be incorrect
9393

94-
ERA001 Found commented-out code
94+
ERA001 [*] Found commented-out code
9595
--> ERA001.py:21:5
9696
|
9797
19 | class A():
@@ -109,7 +109,7 @@ help: Remove commented-out code
109109
23 | dictionary = {
110110
note: This is a display-only fix and is likely to be incorrect
111111

112-
ERA001 Found commented-out code
112+
ERA001 [*] Found commented-out code
113113
--> ERA001.py:26:5
114114
|
115115
24 | dictionary = {
@@ -129,7 +129,7 @@ help: Remove commented-out code
129129
28 |
130130
note: This is a display-only fix and is likely to be incorrect
131131

132-
ERA001 Found commented-out code
132+
ERA001 [*] Found commented-out code
133133
--> ERA001.py:27:5
134134
|
135135
25 | # "key1": 123, # noqa: ERA001
@@ -148,7 +148,7 @@ help: Remove commented-out code
148148
29 | #import os # noqa
149149
note: This is a display-only fix and is likely to be incorrect
150150

151-
ERA001 Found commented-out code
151+
ERA001 [*] Found commented-out code
152152
--> ERA001.py:32:1
153153
|
154154
30 | #import os # noqa
@@ -168,7 +168,7 @@ help: Remove commented-out code
168168
34 | # try: print()
169169
note: This is a display-only fix and is likely to be incorrect
170170

171-
ERA001 Found commented-out code
171+
ERA001 [*] Found commented-out code
172172
--> ERA001.py:33:1
173173
|
174174
32 | # case 1:
@@ -187,7 +187,7 @@ help: Remove commented-out code
187187
35 | # except:
188188
note: This is a display-only fix and is likely to be incorrect
189189

190-
ERA001 Found commented-out code
190+
ERA001 [*] Found commented-out code
191191
--> ERA001.py:34:1
192192
|
193193
32 | # case 1:
@@ -207,7 +207,7 @@ help: Remove commented-out code
207207
36 | # except Foo:
208208
note: This is a display-only fix and is likely to be incorrect
209209

210-
ERA001 Found commented-out code
210+
ERA001 [*] Found commented-out code
211211
--> ERA001.py:35:1
212212
|
213213
33 | # try:
@@ -227,7 +227,7 @@ help: Remove commented-out code
227227
37 | # except Exception as e: print(e)
228228
note: This is a display-only fix and is likely to be incorrect
229229

230-
ERA001 Found commented-out code
230+
ERA001 [*] Found commented-out code
231231
--> ERA001.py:36:1
232232
|
233233
34 | # try: # with comment
@@ -247,7 +247,7 @@ help: Remove commented-out code
247247
38 |
248248
note: This is a display-only fix and is likely to be incorrect
249249

250-
ERA001 Found commented-out code
250+
ERA001 [*] Found commented-out code
251251
--> ERA001.py:37:1
252252
|
253253
35 | # try: print()
@@ -266,7 +266,7 @@ help: Remove commented-out code
266266
39 |
267267
note: This is a display-only fix and is likely to be incorrect
268268

269-
ERA001 Found commented-out code
269+
ERA001 [*] Found commented-out code
270270
--> ERA001.py:38:1
271271
|
272272
36 | # except:
@@ -284,7 +284,7 @@ help: Remove commented-out code
284284
40 | # Script tag without an opening tag (Error)
285285
note: This is a display-only fix and is likely to be incorrect
286286

287-
ERA001 Found commented-out code
287+
ERA001 [*] Found commented-out code
288288
--> ERA001.py:44:1
289289
|
290290
43 | # requires-python = ">=3.11"
@@ -303,7 +303,7 @@ help: Remove commented-out code
303303
46 | # ]
304304
note: This is a display-only fix and is likely to be incorrect
305305

306-
ERA001 Found commented-out code
306+
ERA001 [*] Found commented-out code
307307
--> ERA001.py:47:1
308308
|
309309
45 | # "requests<3",
@@ -322,7 +322,7 @@ help: Remove commented-out code
322322
49 | # Script tag (OK)
323323
note: This is a display-only fix and is likely to be incorrect
324324

325-
ERA001 Found commented-out code
325+
ERA001 [*] Found commented-out code
326326
--> ERA001.py:75:1
327327
|
328328
73 | # /// script
@@ -342,7 +342,7 @@ help: Remove commented-out code
342342
77 | # ]
343343
note: This is a display-only fix and is likely to be incorrect
344344

345-
ERA001 Found commented-out code
345+
ERA001 [*] Found commented-out code
346346
--> ERA001.py:78:1
347347
|
348348
76 | # "requests<3",

0 commit comments

Comments
 (0)