Skip to content
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

ruff server: Implement quick fix code actions to ignore a diagnostic with # noqa: #11085

Closed
wants to merge 6 commits into from

Conversation

snowsignal
Copy link
Contributor

Summary

Fixes #10594.

Note: this PR is in draft because the actual code to generate noqa comment edits has yet to be implemented. So far, the existing code just sets things up to accept these noqa comment edits.

Test Plan

Will be created after this PR gets taken out of draft.

@snowsignal snowsignal added the server Related to the LSP server label Apr 22, 2024
@snowsignal snowsignal added this to the Ruff Server: Beta milestone Apr 22, 2024
Copy link
Contributor

github-actions bot commented Apr 22, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link

codspeed-hq bot commented Apr 28, 2024

CodSpeed Performance Report

Merging #11085 will not alter performance

Comparing jane/server/noqa-comments (0c40149) with main (349a4cf)

Summary

✅ 30 untouched benchmarks

@snowsignal snowsignal marked this pull request as ready for review May 3, 2024 02:19
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

I think it would be useful to have two separate PRs or commits (whichever you prefer) - one which updates the add_noqa API on the linter side and the other which uses it in the server for the code action. This would be helpful in understanding the motivation behind the change and also allow us to look at them in isolation. I could even check if I can plug the new API for Jupyter Notebook :)

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Would you mind expanding your PR summary with some more detail about what the change is about and add a test plan for both ruff CLI and the vs code extenion.

@snowsignal
Copy link
Contributor Author

I think it would be useful to have two separate PRs or commits (whichever you prefer) - one which updates the add_noqa API on the linter side and the other which uses it in the server for the code action. This would be helpful in understanding the motivation behind the change and also allow us to look at them in isolation. I could even check if I can plug the new API for Jupyter Notebook :)

Good idea, I'll split this PR into two separate PRs.

@snowsignal snowsignal closed this May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Related to the LSP server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide code action to ignore the diagnostic (via noqa)
3 participants