-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Move diff rendering to ruff_db
#20006
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.
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
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
Summary -- This is a preparatory PR in support of #19919. It moves our `Diff` rendering code from `ruff_linter` to `ruff_db`, where we have direct access to the `DiagnosticStylesheet` used by our other diagnostic rendering code. As shown by the tests, this shouldn't cause any visible changes. The colors aren't exactly the same, as I note in a TODO comment, but I don't think there's any existing way to see those, even in tests. The `Diff` implementation is mostly unchanged. I just switched from a Ruff-specific `SourceFile` to a `DiagnosticSource` (removing an `expect_ruff_source_file` call) and updated the `LineStyle` struct and other styling calls to use `fmt_styled` and our existing stylesheet. In support of these changes, I added three styles to our stylesheet: `insertion` and `deletion` for the corresponding diff operations, and `underline`, which apparently we _can_ use, as I hoped on Discord. This isn't supported in all terminals, though. It worked in ghostty but not in st for me. I moved the `calculate_print_width` function from the now-deleted `diff.rs` to a method on `OneIndexed`, where it was available everywhere we needed it. I'm not sure if that's desirable, or if my other changes to the function are either (using `ilog10` instead of a loop). This does make it `const` and slightly simplifies things in my opinion, but I'm happy to revert it if preferred. I also inlined a version of `show_nonprinting` from the `ShowNonprinting` trait in `ruff_linter`: https://github.com/astral-sh/ruff/blob/f4be05a83be770c8c9781088508275170396b411/crates/ruff_linter/src/text_helpers.rs#L3-L5 This trait is now only used in `source_kind.rs`, so I'm not sure it's worth having the trait or the macro-generated implementation (which is only called once). This is obviously closely related to our unprintable character handling in diagnostic rendering, but the usage seems different enough not to try to combine them. https://github.com/astral-sh/ruff/blob/f4be05a83be770c8c9781088508275170396b411/crates/ruff_db/src/diagnostic/render.rs#L990-L998 We could also move the trait to another crate where we can use it in `ruff_db` instead of inlining here, of course. Finally, this PR makes `TextEmitter` a very thin wrapper around a `DisplayDiagnosticsConfig`. It's still used in a few places, though, unlike the other emitters we've replaced, so I figured it was worth keeping around. It's a pretty nice API for setting all of the options on the config and then passing that along to a `DisplayDiagnostics`. Test Plan -- Existing snapshot tests with diffs
4792310 to
78b548a
Compare
|
Summary -- I noticed while working on #20006 that we had a custom `unwrap` function for `Option`. This has been const on stable since 1.83 (docs, release notes), so I think it's safe to use now. I grepped a bit for related todos and found this one for `AsciiCharSet` but no others. Test Plan -- Existing tests
MichaReiser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit tough to review because the moves the code and changes the code are in the same commit but I don't think it's worth the work to split the commits now.
| self.0.get().checked_sub(rhs.get()).and_then(Self::new) | ||
| } | ||
|
|
||
| /// Calculate the length of the string representation of `self`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand what this method does :sweat. I'd find it helpful if it had a few doctests to demonstrate what it does.
The name width is also somewhat misleading because it suggests to me that it computes the unicode width, which it doesn't (but, as I said, I don't understand what it does).
If I understand this correctly, this computes the number of digits? Maybe digits or digits_len would be better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a good reason to put it back to the old version:
ruff/crates/ruff_linter/src/message/diff.rs
Lines 190 to 202 in 1a38831
| /// Calculate the length of the string representation of `value` | |
| pub(super) fn calculate_print_width(mut value: OneIndexed) -> NonZeroUsize { | |
| const TEN: OneIndexed = OneIndexed::from_zero_indexed(9); | |
| let mut width = OneIndexed::ONE; | |
| while value >= TEN { | |
| value = OneIndexed::new(value.get() / 10).unwrap_or(OneIndexed::MIN); | |
| width = width.checked_add(1).unwrap(); | |
| } | |
| width | |
| } |
But yes, it just computes the number of digits. I can add doctests and rename it either way.
Sorry about not splitting the commits!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. I think it's fine on OneIndexed. It's just the name that I found confusing. I think digits would be easier to understand.
Could we use this also in
| let max_index_len = completions.len().saturating_sub(1).to_string().len(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant the old implementation in the new method, but either works for me. I renamed and expanded the docs.
It looks like we can use it there in ty too! Is it okay to unwrap since we know completions is non-empty?
let max_index_len = OneIndexed::new(completions.len()).unwrap().digits().get();Otherwise we can use OneIndexed::from_zero_indexed, but that adds 1 after saturating_sub subtracted one, so it seems a little silly. We can also unwrap_or_default.
Summary -- I noticed while working on #20006 that we had a custom `unwrap` function for `Option`. This has been const on stable since 1.83 ([docs](https://doc.rust-lang.org/std/option/enum.Option.html#method.unwrap), [release notes](https://blog.rust-lang.org/2024/11/28/Rust-1.83.0/)), so I think it's safe to use now. I grepped a bit for related todos and found this one for `AsciiCharSet` but no others. Test Plan -- Existing tests
Summary -- This is a preparatory PR in support of astral-sh#19919. It moves our `Diff` rendering code from `ruff_linter` to `ruff_db`, where we have direct access to the `DiagnosticStylesheet` used by our other diagnostic rendering code. As shown by the tests, this shouldn't cause any visible changes. The colors aren't exactly the same, as I note in a TODO comment, but I don't think there's any existing way to see those, even in tests. The `Diff` implementation is mostly unchanged. I just switched from a Ruff-specific `SourceFile` to a `DiagnosticSource` (removing an `expect_ruff_source_file` call) and updated the `LineStyle` struct and other styling calls to use `fmt_styled` and our existing stylesheet. In support of these changes, I added three styles to our stylesheet: `insertion` and `deletion` for the corresponding diff operations, and `underline`, which apparently we _can_ use, as I hoped on Discord. This isn't supported in all terminals, though. It worked in ghostty but not in st for me. I moved the `calculate_print_width` function from the now-deleted `diff.rs` to a method on `OneIndexed`, where it was available everywhere we needed it. I'm not sure if that's desirable, or if my other changes to the function are either (using `ilog10` instead of a loop). This does make it `const` and slightly simplifies things in my opinion, but I'm happy to revert it if preferred. I also inlined a version of `show_nonprinting` from the `ShowNonprinting` trait in `ruff_linter`: https://github.com/astral-sh/ruff/blob/f4be05a83be770c8c9781088508275170396b411/crates/ruff_linter/src/text_helpers.rs#L3-L5 This trait is now only used in `source_kind.rs`, so I'm not sure it's worth having the trait or the macro-generated implementation (which is only called once). This is obviously closely related to our unprintable character handling in diagnostic rendering, but the usage seems different enough not to try to combine them. https://github.com/astral-sh/ruff/blob/f4be05a83be770c8c9781088508275170396b411/crates/ruff_db/src/diagnostic/render.rs#L990-L998 We could also move the trait to another crate where we can use it in `ruff_db` instead of inlining here, of course. Finally, this PR makes `TextEmitter` a very thin wrapper around a `DisplayDiagnosticsConfig`. It's still used in a few places, though, unlike the other emitters we've replaced, so I figured it was worth keeping around. It's a pretty nice API for setting all of the options on the config and then passing that along to a `DisplayDiagnostics`. Test Plan -- Existing snapshot tests with diffs
Summary
This is a preparatory PR in support of #19919. It moves our
Diffrendering code fromruff_lintertoruff_db, where we have direct access to theDiagnosticStylesheetused by our other diagnostic rendering code. As shown by the tests, this shouldn't cause any visible changes. The colors aren't exactly the same, as I note in a TODO comment, but I don't think there's any existing way to see those, even in tests.The
Diffimplementation is mostly unchanged. I just switched from a Ruff-specificSourceFileto aDiagnosticSource(removing anexpect_ruff_source_filecall) and updated theLineStylestruct and other styling calls to usefmt_styledand our existing stylesheet.In support of these changes, I added three styles to our stylesheet:
insertionanddeletionfor the corresponding diff operations, andunderline, which apparently we can use, as I hoped on Discord. This isn't supported in all terminals, though. It worked in ghostty but not in st for me.I moved the
calculate_print_widthfunction from the now-deleteddiff.rsto a method onOneIndexed, where it was available everywhere we needed it. I'm not sure if that's desirable, or if my other changes to the function are either (usingilog10instead of a loop). This does make itconstand slightly simplifies things in my opinion, but I'm happy to revert it if preferred.I also inlined a version of
show_nonprintingfrom theShowNonprintingtrait inruff_linter:ruff/crates/ruff_linter/src/text_helpers.rs
Lines 3 to 5 in f4be05a
This trait is now only used in
source_kind.rs, so I'm not sure it's worth having the trait or the macro-generated implementation (which is only called once). This is obviously closely related to our unprintable character handling in diagnostic rendering, but the usage seems different enough not to try to combine them.ruff/crates/ruff_db/src/diagnostic/render.rs
Lines 990 to 998 in f4be05a
We could also move the trait to another crate where we can use it in
ruff_dbinstead of inlining here, of course.Finally, this PR makes
TextEmittera very thin wrapper around aDisplayDiagnosticsConfig. It's still used in a few places, though, unlike the other emitters we've replaced, so I figured it was worth keeping around. It's a pretty nice API for setting all of the options on the config and then passing that along to aDisplayDiagnostics.Test Plan
Existing snapshot tests with diffs