Make rendering of fix diffs more concise - #26161
Conversation
AlexWaygood
added
ty
Memory usage reportSummary
Significant changesClick to expand detailed breakdownprefect
trio
sphinx
|
|
|
317ed58 to
42ee9cf
Compare
|
Reducing the context size makes sense to me, but I'm not sure if 0 is ideal. Let's say you have something like this if a in (
"a",
"b",
"a",
"c"
): ...Ruff flags the duplicate "a" in the set. I think the diagnostic itself is understandable (or at least, could be made that way) by highlighting both occurrences of "a" (it might still be somewhat difficult with context window 0, but at least you have the line numbers). But the fix with a context window now shows you one line that deletes "a". You won't be able to tell whether it deletes the first or second "a" without cross referencing with the diagnostic code frame. That's why I'm leaning towards retaining at least 1 line of context. It might also be helpful to make the same change in Ruff and publish the snapshot diff somewhere. Ruff's coverage is much better. |
| unformatted: File would be reformatted | ||
| --> input.py:1:1 | ||
| | | ||
| - |
There was a problem hiding this comment.
E.g. this diff is not very useful anymore. We could as well omit it
| | | ||
| help: Remove unused import: `os` | ||
| | | ||
| - import os # F401 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
It results in a huuuuge diff on this PR, so I got scared and backed out that change 😆 but this makes sense |
|
(And yep, I'll bump the context window to 1) |
5929e7c to
0e4ed0c
Compare
0e4ed0c to
22df38a
Compare
|
I bumped the context window to 1. I also added a mechanism similar to #24694 so that same-annotation fix diffs are merged if they are <=2 lines apart. Here's the new before/after for the diagnostic in the PR description. Before
On this PR
|
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
I like the context of 1. I haven't looked through the entire diff, but I noticed a few extra empty lines that aren't clear to me where they come from
| - print( 'hello' ) | ||
| 5 + print("hello") | ||
| 6 | ``` | ||
| | |
There was a problem hiding this comment.
Why are we now retaining more lines here
There was a problem hiding this comment.
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
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
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
AlexWaygood
changed the title
3 participants
Summary
On
main:Screenshot
with this PR:
Screenshot
This makes our rendering of fix diffs more similar to how we render annotations. It's also more similar to rustc fix suggestions:
Screenshot
The functional changes in this PR are in
ruff_db. Everything else is snapshot changes.Test Plan
I added a test