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

Generate Fix for suppressing violations #4307

Open
MichaReiser opened this issue May 9, 2023 · 7 comments
Open

Generate Fix for suppressing violations #4307

MichaReiser opened this issue May 9, 2023 · 7 comments
Labels
core Related to core functionality

Comments

@MichaReiser
Copy link
Member

This is not fully-fledged, it's more a description of an idea that I would like to explore.

Currently, Ruff serializes the noqa_row as part of the JSON messages and the VS code plugin uses that row number to insert the noqa comment at the right place. The issue that I have with the noqa_row is that it leaks internal details and complicates the LSP because it has to know how to correctly add a noqa comment.

What if ruff would instead generate a second Fix to add the noqa suppression comment?

@bluetech
Copy link
Contributor

bluetech commented May 9, 2023

I had the same thought: astral-sh/ruff-lsp#76 (comment)

It does feel much cleaner, however these are not regular fixes (+ it makes every lint "fixable"...) so should be distinguished in some way - how to do it?

@charliermarsh charliermarsh added the core Related to core functionality label May 9, 2023
@MichaReiser
Copy link
Member Author

MichaReiser commented May 9, 2023

I had the same thought: astral-sh/ruff-lsp#76 (comment)

Oh nice, I didn't know about this. Thanks for cross-referencing!

It does feel much cleaner, however these are not regular fixes (+ it makes every lint "fixable"...) so should be distinguished in some way - how to do it?

Hmm, that's a good point. I guess we could either:

  • Use the newly introduced Fix::applicability and only show violations as fixable if they have at least one non-manual fix.
  • This would be harder to do in Ruff today because it doesn't support this capability, but we could provide this as a code action if the cursor is positioned in a range with a violation.

@bluetech
Copy link
Contributor

I was thinking, since this is not useful for regular cli usage, but only for LSP-like scenarios, that the LSP should just tell Ruff "please generate these", in one way or another.

@T-256
Copy link
Contributor

T-256 commented Jan 4, 2024

Since it'd be used mostly by LSP, I'd recommend hidden CLI option --suggest-noqa.

@dhruvmanila
Copy link
Member

\cc @snowsignal You might be interested in this as it would easier to implement in the native LSP implementation.

@MichaReiser
Copy link
Member Author

@snowsignal do you think this issue is still relevant after your new LSP implementation?

@snowsignal
Copy link
Contributor

@MichaReiser I think this is still relevant because we still need to support noqa suppression comments in the new extension, which calls into ruff_linter directly. We'll still need to figure out how to generate fixes for noqa comments, though it would probably be a separate method called after the linter is run.

Re: #4307 (comment)

This would be harder to do in Ruff today because it doesn't support this capability, but we could provide this as a code action if the cursor is positioned in a range with a violation.

With the new Ruff LSP, we should be able to support this, though it will require the implementation of diagnostic caching first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Related to core functionality
Projects
None yet
Development

No branches or pull requests

6 participants