Set severity for non-rule diagnostics #21559
Conversation
crates/ruff_server/src/lint.rs
Outdated
| ) | ||
| } else { | ||
| (None, None, None) | ||
| let severity = match diagnostic.severity() { |
There was a problem hiding this comment.
Thank you. Hmm, this seems like an oversight to me from our diagnostic refactor. @ntBre could we always use diagnostic.severity here?
There was a problem hiding this comment.
Yeah it looks like it to me, it does seem like an oversight because I couldn't use the severity helper.
There was a problem hiding this comment.
Oh no we can't because all diagnostics should remain errors on the CLI, but we want to reduce the severity of some errors to warnings in the editor only
There was a problem hiding this comment.
Oh right, yes. I wasn't sure if you meant could we have used the diagnostic.severity() in this branch all along (which is what I answered), or if we could just use diagnostic.severity() for both cases now. I think the changes in this PR make sense to me since we do the remapping you said.
There was a problem hiding this comment.
I would do a small refactor here to also show the diagnostic id in case the secondary code is missing. This should also allow us to make the two code paths more similar
let (severity, code) = if let Some(code) = code {
(severity(code), code))
} else {
(
match diagnostic.severity { ... },
&diagnostic.id
)
};
let tags = tags(diagnostics); // or inline into the struct construction below
let code = lsp_types::NumberOrString::String(code.to_string());
senekor
force-pushed
the
senekor/lmkxkqrssyst
branch
from
6ab18ed to
e5bfbc1
Compare
MichaReiser
changed the title
|
…d-typevar * origin/main: (24 commits) [ty] Remove brittle constraint set reveal tests (#21568) [`ruff`] Catch more dummy variable uses (`RUF052`) (#19799) [ty] Use the same snapshot handling as other tests (#21564) [ty] suppress autocomplete suggestions during variable binding (#21549) Set severity for non-rule diagnostics (#21559) [ty] Add `with_type` convenience to display code (#21563) [ty] Implement docstring rendering to markdown (#21550) [ty] Reduce indentation of `TypeInferenceBuilder::infer_attribute_load` (#21560) Bump 0.14.6 (#21558) [ty] Improve debug messages when imports fail (#21555) [ty] Add support for relative import completions [ty] Refactor detection of import statements for completions [ty] Use dedicated collector for completions [ty] Attach subdiagnostics to `unresolved-import` errors for relative imports as well as absolute imports (#21554) [ty] support PEP 613 type aliases (#21394) [ty] More low-hanging fruit for inlay hint goto-definition (#21548) [ty] implement `TypedDict` structural assignment (#21467) [ty] Add more random TypeDetails and tests (#21546) [ty] Add goto for `Unknown` when it appears in an inlay hint (#21545) [ty] Add type definitions for `Type::SpecialForm`s (#21544) ...
3 participants
Summary
This change makes ruff server emit LSP diagnostics with severity level in more situations. Specifically, if the diagnostic didn't have a "secondary code" (I'm not super familiar what that is.) ruff server would previously emit no severity.
This isn't good, because we always have an internal severity, so the information is right there. Editor experience is degraded without the severity information. For example, I use Helix. Themes typically don't consider the case of diagnostics without severity, and the fallback style is very ugly on most themes. Another functional problem is that Helix has a "diagnostics picker", which can filter by severity. This allows users to focus on errors before warnings. It doesn't work without the severity information.
I also added
tags(diagnostic)to the same code path, because the information also doesn't seem to depend on the "secondary code". However, I haven't noticed this information missing in my own use of ruff, so I didn't confirm the difference in my own testing.Test Plan
I saw diagnostics without severity in my editor (Helix), made the fix, installed ruff, restarted LSP in Helix, confirmed that diagnostics now have severity.
A very simple file to confirm this just contains
if, which will emit a "Expected an expression" diagnostic. With this PR, it has the severity "error", without it it has none.