Skip to content

[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

Merged
merged 2 commits into from
May 3, 2025

Conversation

ercbot
Copy link
Contributor

@ercbot ercbot commented Apr 27, 2025

Summary

closes #17536

  • Adds DiagnosticTag enum { Unnecessary, Deprecated } and a tags fields to the Annotation struct in the ruff diagnostics system
  • Updates the to_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

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Apr 27, 2025
Copy link
Contributor

github-actions bot commented Apr 27, 2025

mypy_primer results

No ecosystem changes detected ✅

@MichaReiser MichaReiser requested a review from BurntSushi April 28, 2025 06:12
@MichaReiser
Copy link
Member

Nice!

Existing tests have passed, seeking guidance on where to add new tests for this if necessary

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 DEPRECATED or UNNECESSARY tag)

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ercbot ercbot marked this pull request as ready for review April 29, 2025 03:23
@MichaReiser
Copy link
Member

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.

@AlexWaygood AlexWaygood removed their request for review April 29, 2025 06:40
@dhruvmanila
Copy link
Member

If you're using VS Code, you can use the existing Ruff extension to test out the red-knot server by updating ruff.path to point to the red_knot binary:

{
	"ruff.path": ["/path/to/red_knot"]
}

@MichaReiser
Copy link
Member

I went ahead and rebased this PR, added methods to set the tags on Diagnostic and tested that the tag is propagated to the client by adding it to DIVISION_BY_ZERO (see how the code is now grayed out in VS code)

Screenshot 2025-05-03 at 20 25 01

@MichaReiser MichaReiser force-pushed the eb/diagnostic-tag branch from ff4fbf8 to 6e2d1c7 Compare May 3, 2025 18:27
@MichaReiser MichaReiser merged commit 8535af8 into astral-sh:main May 3, 2025
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[red-knot] Add support for the LSP diagnostic tag
5 participants