-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Report diagnostics for invalid/unmatched range suppression comments #21908
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
282450e to
7e9def9
Compare
|
|
Uff, |
## Summary Ignores `#ruff:isort` when parsing suppressions similar to `#ruff:noqa`. Should clear up ecosystem issues in #21908 ## Test Plan cargo tests
100ed3f to
999a481
Compare
MichaReiser
left a comment
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.
Overall this looks good. A few more smaller suggestions
crates/ruff_linter/src/rules/ruff/rules/invalid_suppression_comment.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unmatched_suppression_comment.rs
Outdated
Show resolved
Hide resolved
...uff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__range_suppressions.snap
Outdated
Show resolved
Hide resolved
...uff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__range_suppressions.snap
Outdated
Show resolved
Hide resolved
| }, | ||
| range, | ||
| ); | ||
| diagnostic.set_fix(Fix::safe_edit(edit)); |
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.
Does noqa offer a fix if the rule code is invalid? Removing is one possible solution but the user might also have misspelled the rule name and the proper fix is to fix the rule name instead.
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.
Yes, the noqa implementation does a range deletion of either the entire comment or the individual invalid code, and also marks those as safe edits.
| suppression.comments.len() == 1 | ||
| && suppression.comments[0].action == SuppressionAction::Disable |
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.
I think this is another case where I'd prefer to have dedicated enable and disable fields on Suppression. It would remove the need for code like this to "poke" into the internals.
...uff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__range_suppressions.snap
Outdated
Show resolved
Hide resolved
ntBre
left a comment
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.
Nice! Just a couple of small things from me beyond Micha's comments.
This is a bit off topic, and I'm not sure if y'all have discussed this (or already implemented it?), but I was thinking it would be nice to have a code action to wrap the selected region with these comments, like we have for adding a noqa comment.
| /// ## References | ||
| /// - [Ruff error suppression](https://docs.astral.sh/ruff/linter/#error-suppression) | ||
| #[derive(ViolationMetadata)] | ||
| #[violation_metadata(preview_since = "0.14.9")] |
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.
We'll have to bump these after the release this week.
I think this is trickier because the main use case is to suppress multiple statements. It still recommended to use noqa for local suppressions. Showing too many code actions is also noisy, which is why I wouldn't implement this for now |
|
Checkpointing the feedback I got to today. |
8a54e01 to
5bdf049
Compare
|
Reorganized the logic to reduce repeat iterations and only consider each suppression for one type of violation at a time (I think that's what Micha was getting at?). I'd like to leave consolidating the diagnostics and any other refactors/structural changes to follow-up PRs. |
MichaReiser
left a comment
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.
A few more nits about which diagnostics we emit but this otherwise looks good.
...uff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__range_suppressions.snap
Show resolved
Hide resolved
| 94 | # ruff: disable[F841, RQW320] | ||
| 95 | value = 0 | ||
| | | ||
| help: Remove the rule code |
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.
Maybe: Remove the suppression comment if all codes are invalid
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.
Changing this would also change the help text for all #noqa directives as well. Should that happen in a separate PR?
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.
Yeah, maybe
Summary
Test Plan
Updated snapshots from test cases with unmatched suppression comments
Issue #3711
Fixes #21878
Fixes #21875