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

Add check for is comparison with mutable initialisers to rule F632 #8607

Merged
merged 3 commits into from
Nov 11, 2023

Conversation

jesse1412
Copy link
Contributor

Summary

Adds an extra check to F632 to check for any is comparisons to a mutable initialisers.
Implements #8589 .

Example:

named_var = {}
if named_var is {}:  # F632 (fix)
    pass

The if condition will always evaluate to False because it checks on identity and it's impossible to take the same identity as a hard coded list/set/dict initializer.

Multiple test cases were added to ensure the rule works + doesn't flag false positives + the fix works correctly.

Copy link
Contributor

github-actions bot commented Nov 10, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@vinhocent
Copy link
Contributor

Just wondering - what was the original reasoning for not using a match statement in is_constant() @charliermarsh ?

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Nov 10, 2023
@zanieb
Copy link
Member

zanieb commented Nov 10, 2023

We should probably gate this change to only apply when preview mode is enabled per the versioning policy as I think it fits into

The scope of a stable rule is significantly increased

Note on how to do this in another pull request #8608 (comment)

@zanieb
Copy link
Member

zanieb commented Nov 10, 2023

Thanks for contributing :) the rest of the implementation looks good to me. I don't have the answer to your question though.

@jesse1412
Copy link
Contributor Author

I thiiink I've added the preview flag. And thanks for the encouraging words!

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) November 11, 2023 00:20
@charliermarsh charliermarsh added the preview Related to preview mode features label Nov 11, 2023
@charliermarsh charliermarsh merged commit 39728a1 into astral-sh:main Nov 11, 2023
16 checks passed
@jesse1412 jesse1412 deleted the is-comp-mutable-initializer branch November 13, 2023 13:10
charliermarsh added a commit that referenced this pull request Jun 26, 2024
…632`) (#12049)

## Summary

See: #8607. Rare but
uncontroversial.
MichaReiser pushed a commit that referenced this pull request Jun 27, 2024
…632`) (#12049)

## Summary

See: #8607. Rare but
uncontroversial.
MichaReiser pushed a commit that referenced this pull request Jun 27, 2024
…632`) (#12049)

## Summary

See: #8607. Rare but
uncontroversial.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants