Allow hiding the diagnostic severity in ruff_db#19644
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| invalid-syntax: | 29 | 29 | 0 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+29 -0 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)
openai/openai-cookbook (+29 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select A,E703,F704,B015,B018,D100
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 12:1:8: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 21:1:11: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 21:1:19: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 21:1:50: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:10:11: invalid-syntax: Expected ',', found name
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:10:16: invalid-syntax: Expected ',', found '='
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:10:22: invalid-syntax: Expected ',', found name
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:10:24: invalid-syntax: Expected ',', found ';'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:11:27: invalid-syntax: Expected ',', found '='
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:11:34: invalid-syntax: Expected ',', found name
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:11:48: invalid-syntax: Expected ',', found ';'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:11:49: invalid-syntax: Expected '}', found NonLogicalNewline
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:12:1: invalid-syntax: Unexpected indentation
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:13:3: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:13:4: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:13:5: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:13:6: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:14:1: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:14:2: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:4:7: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:5:14: invalid-syntax: Expected ':', found '{'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:6:25: invalid-syntax: Expected ',', found '='
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:6:46: invalid-syntax: Expected ',', found ';'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:6:47: invalid-syntax: Expected '}', found newline
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:6:9: invalid-syntax: Expected ',', found '{'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:7:13: invalid-syntax: Expected ':', found 'break'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:7:1: invalid-syntax: Unexpected indentation
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:8:28: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:9:18: invalid-syntax: Expected an expression
Changes by rule (1 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| invalid-syntax: | 29 | 29 | 0 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
|
I'll take a closer look tomorrow but my preference for |
|
I thought it would be a bit repetitive to render |
|
Can't we remove the |
|
Yes, but that will also change the That's exactly what we should do as soon as we can break concise diagnostics for syntax errors, though. |
|
I guess we could move the Edit: after trying this, we'd actually have to move the prefix into every output format except |
I'm sort of fine if they change. They should all contain the lint id and the message, so that the prefix seems unnecessary |
MichaReiser
left a comment
There was a problem hiding this comment.
I now took a closer look. I'd prefer to avoid the invalid syntax special casing. An empty id feels hacky. I'd simply make invalid syntax an error like every other one (except that we use the diagnostic.id instead of the secondary_code as we should for all errors without a secondary code)
|
That totally makes sense to me, I was just worried about compatibility. Is this like what you had in mind? That's after unwrapping to the I'd prefer having a colon there ( but not so much with the fix indicator: Eh, maybe that's not so bad. What do you think? |
|
I do like the colon! But I don't think we should make the change now as it breaks the concise output format. I think it's fine without a colon and we can create an issue to improve this later |
This one is complicated a bit by the `unique_rules` collection, which excludes syntax errors. I added the new enum to avoid needing to change the definition of `SarifRule::from` for `SecondaryCode`. Initially it felt like a syntax error should also be a unique `SarifRule`, but it doesn't have most of the other fields like `linter`, `summary`, or `url`, and `name` and `code` would be the same.
1e1bd62 to
1cdb26c
Compare
|
Okay I think I've implemented what we discussed here and on Discord for the The The other output formats did not fall back on the The ::error title=Ruff (F821),file=[TMP]/input.py,line=2,col=5,endLine=2,endColumn=6::input.py:2:5: F821 Undefined name `y`
-::error title=Ruff,file=[TMP]/input.py,line=3,col=1,endLine=3,endColumn=6::input.py:3:1: SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
+::error title=Ruff (invalid-syntax),file=[TMP]/input.py,line=3,col=1,endLine=3,endColumn=6::input.py:3:1: invalid-syntax: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)The Initially it felt like a syntax error should also be a unique |
MichaReiser
left a comment
There was a problem hiding this comment.
The snapshot changes look good to me. I'll review the rendering changes tomorrow
| ::error title=Ruff (F401),file=[TMP]/input.py,line=1,col=8,endLine=1,endColumn=10::input.py:1:8: F401 `os` imported but unused | ||
| ::error title=Ruff (F821),file=[TMP]/input.py,line=2,col=5,endLine=2,endColumn=6::input.py:2:5: F821 Undefined name `y` | ||
| ::error title=Ruff,file=[TMP]/input.py,line=3,col=1,endLine=3,endColumn=6::input.py:3:1: SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10) | ||
| ::error title=Ruff (invalid-syntax),file=[TMP]/input.py,line=3,col=1,endLine=3,endColumn=6::input.py:3:1: invalid-syntax: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10) |
There was a problem hiding this comment.
I agree, this feels odd but it is consistent to how we handle the other rules. I'm fine leaving this as is (seems like a separate change)
|
I think the ecosystem check makes sense now. I could make the regex look for either |
|
Thanks for the review! I fixed your comments and also added a small commit (1ae8695) fixing the styling issue I pointed out in the summary ( With that, the diagnostics look quite similar in color to Ruff's current
It's still a bit of a shame about the missing cyan in the fixability marker, but I don't think there's a good way to fix that unless we add an (I temporarily switched |
At this point it will be very hard to every use the upstream crate again because I don't think we can convince them to add this as a feature. But who knows, maybe the have something much more flexible. I think I'd be okay with adding it |
| if annotation.is_fixable { | ||
| buffer.append(line_offset, "[", stylesheet.none); | ||
| buffer.append(line_offset, "*", stylesheet.help); | ||
| buffer.append(line_offset, "] ", stylesheet.none); | ||
| } |
There was a problem hiding this comment.
Do we need to gate this behind hide_severity or could we always render it after the severity/and id (unconditionally) if is_fixable is true
There was a problem hiding this comment.
Oh good point. I don't think we strictly need to gate it, but it would look something like this if we had a fixable ty-style diagnostic right now:
error[invalid-assignment][*]: Object of type `Literal[1]` is not assignable to `str`
I guess that looks pretty good, and it should be unreachable currently, in any case. I'll move it down!
Summary -- This is the other commit I wanted to spin off from #19415, currently stacked on #19644. This PR suppresses blank snippets for empty ranges at the very beginning of a file, and for empty ranges in non-existent files. Ruff includes empty ranges for IO errors, for example. https://github.com/astral-sh/ruff/blob/f4e93b63351bfe035b1fc5a7bc605a52979e7841/crates/ruff_linter/src/message/text.rs#L100-L110 The diagnostics now look like this (new snapshot test): ``` error[test-diagnostic]: main diagnostic message --> example.py:1:1 ``` Instead of [^*] ``` error[test-diagnostic]: main diagnostic message --> example.py:1:1 | | ``` Test Plan -- A new `ruff_db` test showing the expected output format [^*]: This doesn't correspond precisely to the example in the PR because of some details of the diagnostic builder helper methods in `ruff_db`, but you can see another example in the current version of the summary in #19415.
I thought this was unreachable when adding it (#19644 (comment)), but obviously it's tested here! Without the spacing tweak, the snapshots were rendering like this: ``` error[unused-import][*] : `os` imported but unused error[unused-import][*] : `math` imported but unused ``` with awkward extra spaces before the colons.
## Summary Fixes #19881. While I was here, I also made a couple of related tweaks to the output format. First, we don't need to strip the `SyntaxError: ` prefix anymore since that's not added directly to the diagnostic message after #19644. Second, we can use `secondary_code_or_id` to fall back on the lint ID for syntax errors, which changes the `check_name` from `syntax-error` to `invalid-syntax`. And then the main change requested in the issue, prepending the `check_name` to the description. ## Test Plan Existing tests and a new screenshot from GitLab: <img width="362" height="113" alt="image" src="https://github.com/user-attachments/assets/97654ad4-a639-4489-8c90-8661c7355097" />




Summary
This PR is a spin-off from #19415. It enables replacing the severity and lint name in a ty-style diagnostic:
with the noqa code and optional fix availability icon for a Ruff diagnostic:
or nothing at all for a Ruff syntax error, where the previous
SyntaxErrorprefix is replaced with theinvalid-syntaxlint name:Ruff adds the
SyntaxErrorprefix to these messages manually.Initially (d912458), I just passed a
hide_severityflag through a bunch of calls to get it intoannotate-snippets, but after looking at it again today, I think reusing theNoneseverity/level gave a nicer result. As I note in a lengthy code comment, I think all of this code should be temporary and reverted when Ruff gets real severities, so hopefully it's okay if it feels a little hacky.I think the main visible downside of this approach is that we can't style the asterisk in the fix availabilty icon in cyan, as in Ruff's current output. It's part of the message in this PR and any styling gets overwritten in
annotate-snippets.Hmm, I guess reusing
Level::Nonealso means theF401isn't red anymore. Maybe my initial approach was better after all. In any case, the rest of the PR should be basically the same, it just depends how we want to toggle the severity.Test Plan
New
ruff_dbtests. These snapshots should be compared to the two tests just above them (hide_severity_outputvsoutputandhide_severity_syntax_errorsagainstsyntax_errors).