Skip to content

Make rendering of fix diffs more concise #26161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AlexWaygood merged 5 commits into from
Jun 22, 2026
Merged

Make rendering of fix diffs more concise #26161

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
42ee9cf
[ty] Make rendering of fix diffs more concise
AlexWaygood Jun 19, 2026
22df38a
add one line of context and update one hundred billion snapshots
AlexWaygood Jun 20, 2026
efabdbe
Merge diff hunks on the same annotation if they have <=2 lines separa…
AlexWaygood Jun 20, 2026
4ecca7f
Merge remote-tracking branch 'upstream/main' into alex/subdiagnostic-…
AlexWaygood Jun 22, 2026
c8870d7
Use fixed context for fix diffs
AlexWaygood Jun 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
40 changes: 22 additions & 18 deletions crates/ruff/tests/cli/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,31 +615,33 @@ fn output_format_notebook() -> Result<()> {
unformatted: File would be reformatted
--> CRATE_ROOT/resources/test/fixtures/unformatted.ipynb:cell 1:1:1
::: cell 1
|
1 | import numpy
- maths = (numpy.arange(100)**2).sum()
- stats= numpy.asarray([1,2,3,4]).median()
2 +
3 + maths = (numpy.arange(100) ** 2).sum()
4 + stats = numpy.asarray([1, 2, 3, 4]).median()
|
::: cell 3
1 | # A cell with IPython escape command
2 | def some_function(foo, bar):
|
3 | pass
4 +
5 +
6 | %matplotlib inline
::: cell 4
1 | foo = %pwd
- def some_function(foo,bar,):
2 +
3 +
4 + def some_function(
5 + foo,
6 + bar,
7 + ):
8 | # Another cell with IPython escape command
9 | foo = %pwd
10 | print(foo)
|
::: cell 4
|
1 | foo = %pwd
- def some_function(foo,bar,):
2 +
3 +
4 + def some_function(
5 + foo,
6 + bar,
7 + ):
8 | # Another cell with IPython escape command
|

1 file would be reformatted

Expand Down Expand Up @@ -2451,8 +2453,7 @@ fn markdown_formatting_preview_enabled() -> Result<()> {
----- stdout -----
unformatted: File would be reformatted
--> CRATE_ROOT/resources/test/fixtures/unformatted.md:1:1
1 | This is a markdown document with two fenced code blocks:
2 |
|
3 | ```py
- print( "hello" )
- def foo(): pass
Expand All @@ -2471,6 +2472,7 @@ fn markdown_formatting_preview_enabled() -> Result<()> {
14 + def foo():
15 + pass
16 | ```
|

1 file would be reformatted

Expand Down Expand Up @@ -2591,18 +2593,20 @@ print( 'hello' )

unformatted: File would be reformatted
--> test.bar:1:1
2 | Text string
3 |
|
4 | ```py
- print( 'hello' )
5 + print("hello")
6 | ```
|

@MichaReiser MichaReiser Jun 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we now retaining more lines here

@AlexWaygood AlexWaygood Jun 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a 1-row gutter around fix-diff hunks. This is similar to what we do for our diagnostic annotations, and was again inspired by what rustc does for its fix diffs. Here's what rustc does:

rustc diff suggestion image

It allows the fix diff to "breathe" a little bit by adding a little bit of padding below the help message, making the rendering look less cluttered. I found the "cluttered" issue was especially a problem with a small context window. Here's an example diff rendering on this PR currently:

Current rendering with the gutter image

and I could make this change to the PR:

Patch 
diff --git a/crates/ruff_db/src/diagnostic/render/full.rs b/crates/ruff_db/src/diagnostic/render/full.rs
index 1dffd0f65c..88913f675e 100644
--- a/crates/ruff_db/src/diagnostic/render/full.rs
+++ b/crates/ruff_db/src/diagnostic/render/full.rs
@@ -113,15 +113,6 @@ impl<'a> Diff<'a> {
             merge_window: config.merge_window,
         })
     }
-
-    fn write_gutter(&self, f: &mut std::fmt::Formatter, width: NonZeroUsize) -> std::fmt::Result {
-        writeln!(
-            f,
-            "{line} {separator}",
-            line = fmt_styled(Line { index: None, width }, self.stylesheet.line_no),
-            separator = fmt_styled("|", self.stylesheet.line_no),
-        )
-    }
 }

 impl std::fmt::Display for Diff<'_> {
@@ -211,8 +202,6 @@ impl std::fmt::Display for Diff<'_> {
                 writeln!(f, "{:>1$} cell {cell}", ":::", digit_with.get() + 3)?;
             }

-            self.write_gutter(f, digit_with)?;
-
             for (idx, group) in grouped_ops.iter().enumerate() {
                 if idx > 0 {
                     writeln!(f, "{:-^1$}", "-", 80)?;
@@ -273,8 +262,6 @@ impl std::fmt::Display for Diff<'_> {
                     }
                 }
             }
-
-            self.write_gutter(f, digit_with)?;
         }

         match self.fix.applicability() {

which would make the same diagnostic look like this, which I find to still be an improvement over main, but looks a bit cluttered to my eyes:

Rendering without the gutter image


unformatted: File would be reformatted
--> test.foo:1:1
|
-
- print( 'hello' )
1 + print("hello")
|

2 files would be reformatted

Expand Down
4 changes: 4 additions & 0 deletions crates/ruff/tests/cli/lint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3833,7 +3833,9 @@ fn show_fixes_in_full_output_with_preview_enabled() {
| ^^^^
|
help: Remove unused import: `math`
|
Comment thread
MichaReiser marked this conversation as resolved.
- import math
|

Found 1 error.
[*] 1 fixable with the `--fix` option.
Expand Down Expand Up @@ -3897,8 +3899,10 @@ fn rule_panic_mixed_results_full() -> Result<()> {

stable-test-rule-safe-fix: [*] Hey this is a stable test rule with a safe fix.
--> normal.py:1:1
|
1 + # fix from stable-test-rule-safe-fix
2 | import os
|

stable-test-rule-unsafe-fix: Hey this is a stable test rule with an unsafe fix.
--> normal.py:1:1
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff/tests/cli/snapshots/cli__format__output_format_full.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ exit_code: 1
----- stdout -----
unformatted: File would be reformatted
--> input.py:1:1
|
-

@MichaReiser MichaReiser Jun 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. this diff is not very useful anymore. We could as well omit it

1 | from test import say_hy
2 |
3 | if __name__ == "__main__":
|

1 file would be reformatted

Expand Down
4 changes: 2 additions & 2 deletions crates/ruff/tests/cli/snapshots/cli__lint__output_format_preview_full.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ unused-import: [*] `os` imported but unused
3 | match 42: # invalid-syntax
|
help: Remove unused import: `os`
|
- import os # F401

@MichaReiser MichaReiser Jun 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, why are we omitting line numbers now? Oh, is it because the diff shows the line numbers after applying the fix. Hmm, this is somewhat surprising to me (@ntBre). Without line numbers, the diff doesn't feel very useful to me anymore

@ntBre ntBre Jun 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which line number would we display here? I believe we went with the new line number to avoid both the old approach of showing both (e.g. 1 2 |) and to avoid confusing situations like:

1 - import os
1 | x = y

I'd have to look at the code again, but from what I remember I think it might be tricky to detect that we're in this situation (no relevant context and only a deletion).

1 | x = y # F821
2 | match 42: # invalid-syntax
3 | case _: ...
|

undefined-name: Undefined name `y`
--> input.py:2:5
Expand Down
23 changes: 17 additions & 6 deletions crates/ruff_db/src/diagnostic/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,10 +1404,12 @@ pub struct DisplayDiagnosticConfig {
/// here for now as the most "sensible" place for it to live until
/// we had more concrete use cases. ---AG
context: usize,
/// The "merge window" for annotations.
/// The number of unchanged lines to show around each fix diff.
fix_context: usize,
/// The "merge window" for annotations and fix diff hunks.
///
/// If two annotations have fewer than this number of lines between them,
/// they will be merged into a single annotation.
/// Nearby annotations or fix edits are rendered in a single source frame even when their
/// configured context windows would not otherwise overlap.
merge_window: usize,
/// Whether to use preview formatting for Ruff diagnostics.
preview: bool,
Expand Down Expand Up @@ -1436,6 +1438,7 @@ impl DisplayDiagnosticConfig {
color: false,
anonymized_line_numbers: false,
context: 2,
fix_context: 1,
merge_window: 2,
preview: false,
hide_severity: false,
Expand Down Expand Up @@ -1472,10 +1475,18 @@ impl DisplayDiagnosticConfig {
}
}

/// Set the "merge window" for annotations.
/// Set the number of unchanged lines to show around each fix diff.
pub fn fix_context(self, lines: usize) -> DisplayDiagnosticConfig {
DisplayDiagnosticConfig {
fix_context: lines,
..self
}
}

/// Set the "merge window" for annotations and fix diff hunks.
///
/// If two annotations have fewer than this number of lines between them,
/// they will be merged into a single annotation.
/// Nearby annotations or fix edits are rendered in a single source frame even when their
/// configured context windows would not otherwise overlap.
pub fn merge_window(self, lines: usize) -> DisplayDiagnosticConfig {
DisplayDiagnosticConfig {
merge_window: lines,
Expand Down
19 changes: 14 additions & 5 deletions crates/ruff_db/src/diagnostic/render.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2586,12 +2586,15 @@ watermelon
// Default to a merge window of 0 for testing purposes,
// even though this is not the default for user-facing diagnostics.
env.merge_window(0);
// Preserve the historical fix context in existing rendering snapshots. Tests of the
// compact user-facing rendering override this explicitly.
env.fix_context(3);
env
}

/// Set the number of contextual lines to include for each snippet
/// in diagnostic rendering.
fn context(&mut self, lines: usize) {
pub(super) fn context(&mut self, lines: usize) {
// Kind of annoying. I considered making `DisplayDiagnosticConfig`
// be `Copy` (which it could be, at time of writing, 2025-03-07),
// but it seems likely to me that it will grow non-`Copy`
Expand All @@ -2600,11 +2603,17 @@ watermelon
self.config = config.context(lines);
}

/// Set the "merge window" for annotations in this test.
/// Set the number of unchanged lines to include around each fix diff.
pub(super) fn fix_context(&mut self, lines: usize) {
let config = self.config.clone();
self.config = config.fix_context(lines);
}

/// Set the "merge window" for annotations and fix diff hunks in this test.
///
/// If two annotations have fewer than this number of lines between them,
/// they will be merged into a single annotation.
fn merge_window(&mut self, lines: usize) {
/// Nearby annotations or fix edits are rendered in a single source frame even when their
/// configured context windows would not otherwise overlap.
pub(super) fn merge_window(&mut self, lines: usize) {
let config = self.config.clone();
self.config = config.merge_window(lines);
}
Expand Down
Loading
Loading