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.

Sign up for GitHub

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

Jump to bottom

[ruff] Detect duplicate codes as part of unused-noqa (RUF100) #10850

Merged
merged 12 commits into from Apr 27, 2024
Merged

[ruff] Detect duplicate codes as part of unused-noqa (RUF100) #10850

merged 12 commits into from Apr 27, 2024

Conversation

augustelalande
Copy link
Contributor

Summary

Implement duplicate code detection as part of RUF100, mirroring the behavior of flake8-noqa (NQA005) mentioned in #850. The idea to merge the rule into RUF100 was suggested by @MichaReiser #10325 (comment).

Test Plan

Test cases were added to the fixture.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 9, 2024 •
edited

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@augustelalande
Copy link
Contributor Author

Given the violations in the ecosystem check (# noqa: PGH001, S307), there is a question of whether duplicates should be detected based on rule (i.e. PGH001 and S307 are the same rule) or code (i.e. PGH001 and S307 are different codes).

@charliermarsh charliermarsh self-assigned this Apr 10, 2024
@charliermarsh charliermarsh self-requested a review April 10, 2024 04:23
@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Apr 10, 2024
@charliermarsh
Copy link
Member

Hmm yeah... I think it would make sense to match based on rule, but it's confusing that the diagnostic lists the redirected code (and so, presumedly, the one that should stay).

I'd say let's do the duplicate detection based on code for now. Seems simpler.

@augustelalande
Copy link
Contributor Author

augustelalande commented Apr 10, 2024 •
edited

Ok I modified it to use the original code. I also had to modify the list of valid codes, to also use the original code for the fix to work. This has the added effect that redirects will no longer be updated if another rule is violated. But this might be less confusing behavior for users since codes will no longer be updated to new codes without explanation.

If we want to detect redirects and lint against them, then the user should probably be notified that they are using a redirect.

@charliermarsh
Copy link
Member

Could we add a separate "reason" for redirected codes?

@augustelalande
Copy link
Contributor Author

Ya I was just thinking about that. Do you want me to do it in this PR, or a separate one?

@charliermarsh
Copy link
Member

I'm cool with either, whichever is easier for you.

@augustelalande
Copy link
Contributor Author

Ok I will add it to this one

@augustelalande
Copy link
Contributor Author

Should redirects be a seperate rule? Technically RUF100 is unused-noqa which isn't strictly true of redirects.

MichaReiser
MichaReiser approved these changes Apr 11, 2024
View reviewed changes
crates/ruff_linter/src/noqa.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/noqa.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/rules/ruff/rules/redirected_noqa.rs Outdated Show resolved Hide resolved
augustelalande and others added 4 commits April 11, 2024 13:19
@augustelalande @MichaReiser
Use TextRange::at
a19252d 
Co-authored-by: Micha Reiser <micha@reiser.io>
@augustelalande @MichaReiser
Use Some get_redirect_target to trigger violation
4b4cd84 
Co-authored-by: Micha Reiser <micha@reiser.io>
@augustelalande
fmt
55a1dd0
@augustelalande
implement suggestion
e19a50c
@augustelalande augustelalande changed the title [ruff] Detect duplicate codes as part of unused-noqa (RUF100) [ruff] Implement redirected-noqa (RUF101), also detect duplicate codes as part of unused-noqa (RUF100) Apr 11, 2024
@augustelalande
Copy link
Contributor Author

I'm gonna split this to help merge it

@augustelalande augustelalande marked this pull request as draft April 20, 2024 01:44
@augustelalande augustelalande mentioned this pull request Apr 20, 2024
Merged
charliermarsh pushed a commit that referenced this pull request Apr 27, 2024
@augustelalande
[ruff] Implement redirected-noqa (RUF101) (#11052)
5994414 
## Summary

Based on discussion in #10850.

As it stands today `RUF100` will attempt to replace code redirects with
their target codes even though this is not the "goal" of `RUF100`. This
behavior is confusing and inconsistent, since code redirects which don't
otherwise violate `RUF100` will not be updated. The behavior is also
undocumented. Additionally, users who want to use `RUF100` but do not
want to update redirects have no way to opt out.

This PR explicitly detects redirects with a new rule `RUF101` and
patches `RUF100` to keep original codes in fixes and reporting.

## Test Plan

Added fixture.
@augustelalande augustelalande changed the title [ruff] Implement redirected-noqa (RUF101), also detect duplicate codes as part of unused-noqa (RUF100) [ruff] Detect duplicate codes as part of unused-noqa (RUF100) Apr 27, 2024
@augustelalande augustelalande marked this pull request as ready for review April 27, 2024 04:12
@augustelalande
Copy link
Contributor Author

@charliermarsh hopefully this should be an easy merge now 😉

charliermarsh
charliermarsh approved these changes Apr 27, 2024
View reviewed changes
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@charliermarsh charliermarsh enabled auto-merge (squash) April 27, 2024 12:18
@charliermarsh charliermarsh merged commit 2490d2d into astral-sh:main Apr 27, 2024
18 checks passed
@augustelalande augustelalande deleted the ruf100-duplicate-codes branch April 27, 2024 17:10
@BrewTestBot BrewTestBot mentioned this pull request May 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

@charliermarsh charliermarsh charliermarsh approved these changes

Assignees

@charliermarsh charliermarsh

Labels
rule Implementing or modifying a lint rule
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@augustelalande @charliermarsh @MichaReiser