Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Disable E741 in stub files #13119

Merged
merged 3 commits into from
Aug 27, 2024
Merged

Conversation

calumy
Copy link
Contributor

@calumy calumy commented Aug 26, 2024

Summary

As noted in #10569, E741 should not be active in .pyi files as the file author does not control the variable name. Instead, this is deferred to the author of the package for which the stub file is substituting for.

My rust skills are being stretched here. I assume there is a more elegant way of implementing the check checker.source_type != PySourceType::Stub in the ambiguous_variable_name function so that it is only implemented in one place (similar to non_unique_enums); however, I can't get the borrow checker to agree to this.

Test Plan

Added a new test case were E741.py was duplicated and renamed to E741.pyi and a snapshot to confirm that no errors were raised for this new case as E741 should be disabled in .pyi files.

Copy link
Contributor

github-actions bot commented Aug 26, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood AlexWaygood self-assigned this Aug 27, 2024
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks @calumy, this is great! I pushed a few small fixups to your PR:

  1. I made the change preview-only for now; we can stabilise it in Ruff 0.7. Strictly speaking we could probably get away with doing this in a patch release, since it's a reduction in scope rather than an increase in scope -- but it's still a big change in scope, so I feel much safer doing it in preview only first :-)
  2. I added some docs on how it works differently for .pyi files
  3. I made it a bit more DRY by putting the check for whether it's a stub file in the ambiguous_variable_name() function itself, so we don't have to repeat it everywhere.

LMK if you'd like me to explain any of it, very happy to do so :-)

@AlexWaygood AlexWaygood added the preview Related to preview mode features label Aug 27, 2024
@AlexWaygood AlexWaygood merged commit 4e1b289 into astral-sh:main Aug 27, 2024
20 checks passed
@calumy
Copy link
Contributor Author

calumy commented Aug 27, 2024

Thanks @calumy, this is great! I pushed a few small fixups to your PR:

  1. I made the change preview-only for now; we can stabilise it in Ruff 0.7. Strictly speaking we could probably get away with doing this in a patch release, since it's a reduction in scope rather than an increase in scope -- but it's still a big change in scope, so I feel much safer doing it in preview only first :-)
  2. I added some docs on how it works differently for .pyi files
  3. I made it a bit more DRY by putting the check for whether it's a stub file in the ambiguous_variable_name() function itself, so we don't have to repeat it everywhere.

LMK if you'd like me to explain any of it, very happy to do so :-)

All makes sense to me; thanks for laying it out clearly.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
preview Related to preview mode features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants