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

[ruff] Add support for more re patterns (RUF055) #15764

Merged
merged 4 commits into from
Jan 29, 2025

Conversation

Garrett-R
Copy link
Contributor

Summary

Implements some of #14738, by adding support for 6 new patterns:

re.search("abc", s) is None       # ⇒ "abc" not in s
re.search("abc", s) is not None   # ⇒ "abc" in s

re.match("abc", s) is None       # ⇒ not s.startswith("abc")  
re.match("abc", s) is not None   # ⇒ s.startswith("abc")

re.fullmatch("abc", s) is None       # ⇒ s != "abc"
re.fullmatch("abc", s) is not None   # ⇒ s == "abc"

Test Plan

cargo nextest run
cargo insta review

And ran the fix on my startup's repo.

Note

One minor limitation here:

if not re.match('abc', s) is None:
    pass

will get fixed to this (technically correct, just not nice):

if not not s.startswith('abc'):
    pass

This seems fine given that Ruff has this covered: the initial code should be caught by E714 and the fixed code should be caught by SIM208.

Copy link
Contributor

github-actions bot commented Jan 27, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+2 -0 violations, +2 -0 fixes in 2 projects; 53 projects unchanged)

ibis-project/ibis (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ ibis/backends/tests/test_client.py:1080:9: RUF055 [*] Plain string pattern passed to `re` function
+ ibis/backends/tests/test_client.py:1101:9: RUF055 [*] Plain string pattern passed to `re` function

astropy/astropy (+0 -0 violations, +2 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

- astropy/coordinates/tests/test_frames.py:352:11: RUF055 Plain string pattern passed to `re` function
+ astropy/coordinates/tests/test_frames.py:352:11: RUF055 [*] Plain string pattern passed to `re` function

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
RUF055 4 2 0 2 0

@Garrett-R Garrett-R force-pushed the garrett/14738/ruff055-more-patterns branch from d25fa26 to 3b67115 Compare January 27, 2025 06:09
@Garrett-R Garrett-R force-pushed the garrett/14738/ruff055-more-patterns branch from 3b67115 to dc7c258 Compare January 27, 2025 06:22
@Garrett-R Garrett-R marked this pull request as ready for review January 27, 2025 06:30
@AlexWaygood AlexWaygood requested a review from ntBre January 27, 2025 07:45
@AlexWaygood AlexWaygood added rule Implementing or modifying a lint rule preview Related to preview mode features labels Jan 27, 2025
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I suggested some changes, but I think the overall approach is spot-on.

Copy link
Contributor Author

@Garrett-R Garrett-R left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough review! I've applied all suggestions in this commit: 92e84ee

Then I came up with solution to the unsafe fix issue you found in this commit: 3d25918

Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! Just a couple more much smaller changes, and I think this is good to merge.

Copy link
Contributor Author

@Garrett-R Garrett-R left a comment

Choose a reason for hiding this comment

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

Thanks, good ideas! Implemented all those 😎

Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks again for your work on this!

@ntBre ntBre merged commit 6c1e195 into astral-sh:main Jan 29, 2025
21 checks passed
@ntBre
Copy link
Contributor

ntBre commented Jan 29, 2025

Oh and just to acknowledge the not not case, I agree that it's unfortunate, but your observation that the original code should be caught by E714, which I believe is enabled by default, is good enough for me. I think we'd have to look at the grandparent expression to detect this anyway, and I think it's reasonable to draw the line at the first parent, especially in light of the other rules.

@Garrett-R
Copy link
Contributor Author

For sure, thanks for the super helpful review, learned a bunch! 😎

@Garrett-R Garrett-R deleted the garrett/14738/ruff055-more-patterns branch January 29, 2025 19:16
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.

3 participants