Always expand tabs to four spaces in diagnostics#19618
Conversation
|
BurntSushi
left a comment
There was a problem hiding this comment.
This LGTM. You might want Micha to sign-off on the snapshot changes. I don't mind them personally, and I think I might even prefer them. With tab-stops, the number of spaces inserted is variable and could, I think, be confusing as to what is happening. Where as I think if someone is using tabs, if they see they are consistently replaced by 4 space characters, then I think it's probably easier to intuit what's happening.
There was a problem hiding this comment.
I think this is fine. I do prefer the the old layout because it closer matches what users see in their editor. But I don't think it justifies spending much time on.
However, don't we have the same issue with unprintable replacements? Or is that fine, because we only map them to characters that map to the same column? It might be worth adding a comment explaining why it's okay that we do whatever we do ;)
Should this PR also undo the changes introduced in #19535 ?
| | ^^^^^^^^^^^^^^^ E101 | ||
| 15 | print("mixed starts with space") | ||
| | ^^^^^^^^^^^^^^^^^^^^ E101 | ||
| 16 | |
There was a problem hiding this comment.
I took a quick look at what my editor does and the old rendering is closer to what editors render (just noting down)
There was a problem hiding this comment.
That would depend on your configured tabwidth though right? Admittedly, 4 is probably the most common, but if you use a different tabwidth I would guess the rendering would be different?
For the unprintable characters, they don't have a "width": https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=7c5639c10e050961f2ec7b81a82abcd6 But we're replacing it with a character that has a width of 1 column. It's possible this might work fine today if there's a |
Ah, yep, I should revert most of that too. Yeah I guess we do have the same issue for unprintable characters, but it's probably a bit less prominent with a difference of 1 vs 3. And I kind of assumed unprintable characters are less common than tabs too. I'm pretty sure Assuming not, grepping for ruff/crates/ruff_annotate_snippets/src/renderer/display_list.rs Lines 355 to 357 in c5ac998 I think this is for cutting long lines, but just above here is where |
|
Actually, I looked back at the snapshots in #19415, and if the problem is "we display the wrong range in the header," this is okay, at least for the snapshots in our test suite. I'm a bit surprised by this because the I even added a more exaggerated example: nested_fstrings = f'������{f'�{f'�'}'}'where we replace 6 unprintable characters instead of 1 in the So I think our unprintable handling is okay. I'll revert the whitespace parts of #19535! |
|
I also renamed the functions since we're not replacing whitespace anymore, updated the docs on both versions, and finally added a tab test in |
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
632f7b8 to
bc41f16
Compare
Summary
I was a bit stuck on some snapshot differences I was seeing in #19415, but @BurntSushi pointed out that
annotate-snippetsalready normalizes tabs on its own, which was very helpful! Instead of applying this change directly to the other branch, I wanted to try applying it inruff_linterfirst. This should very slightly reduce the number of changes in #19415 proper.It looks like
annotate-snippetsalways expands a tab to four spaces, whereas I think we were aligning to tab stops:6 | spam(ham[1], { eggs: 2}) 7 | #: E201:1:6 - 8 | spam( ham[1], {eggs: 2}) - | ^^^ E201 + 8 | spam( ham[1], {eggs: 2}) + | ^^^^ E201E27.py:15:6: E271 [*] Multiple spaces after keyword | -13 | True and False +13 | True and False 14 | #: E271 15 | a and b | ^^ E271I don't think this is too bad and has the major benefit of allowing us to pass the non-tab-expanded range to
annotate-snippetsin #19415, where it's also displayed in the header. Ruff doesn't have this problem currently because it uses its own concise diagnostic output as the header for full diagnostics, where the pre-expansion range is used directly.Test Plan
Existing tests with a few snapshot updates