-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[red-knot] Add support for the LSP diagnostic tag #17657
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
|
Nice!
We don't really have tests for our diagnostic conversion code. So I thinkit's fine to skip automated tests and instead test this manually (you can change an arbitrary diagnostic to add the |
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.
LGTM
This is great. Would you mind adding a test plan to your PR summary? What I do for features like this is that I record a screen recording demonstrating the feature in VS code. This will require adding the tag to at least one emitted diagnostic. |
If you're using VS Code, you can use the existing Ruff extension to test out the red-knot server by updating {
"ruff.path": ["/path/to/red_knot"]
} |
ff4fbf8
to
6e2d1c7
Compare
Summary
closes #17536
DiagnosticTag enum { Unnecessary, Deprecated }
and a tags fields to the Annotation struct in the ruff diagnostics systemto_lsp_diagnostic
function to include conversion from the above tags to LSP Diagnostic Tags.Note: doesn't update any existing code to use the tags, just wires it up for future implementation
Test Plan
Existing tests have passed, seeking guidance on where to add new tests for this if necessary