Skip to content

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

Merged
ntBre merged 4 commits intoastral-sh:mainfrom
Garrett-R:garrett/14738/ruff055-more-patterns
Jan 29, 2025
Merged

[ruff] Add support for more re patterns (RUF055)#15764
ntBre merged 4 commits intoastral-sh:mainfrom
Garrett-R:garrett/14738/ruff055-more-patterns

Conversation

@Garrett-R
Copy link
Copy Markdown
Contributor

@Garrett-R Garrett-R commented Jan 27, 2025

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.

Garrett-R
Garrett-R commented Jan 27, 2025
View reviewed changes
Comment thread crates/ruff_linter/resources/test/fixtures/ruff/RUF055_0.py
Garrett-R
Garrett-R commented Jan 27, 2025
View reviewed changes
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 27, 2025 •
edited
Loading

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 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
ntBre
ntBre requested changes Jan 27, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread crates/ruff_linter/resources/test/fixtures/ruff/RUF055_2.py Outdated
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Garrett-R
Garrett-R commented Jan 28, 2025
View reviewed changes
Copy link
Copy Markdown
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

Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
ntBre
ntBre reviewed Jan 28, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Garrett-R
Garrett-R commented Jan 29, 2025
View reviewed changes
Copy link
Copy Markdown
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 😎

Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
Comment thread crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs Outdated
ntBre
ntBre approved these changes Jan 29, 2025
View reviewed changes
Copy link
Copy Markdown
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
@ntBre
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Garrett-R commented Jan 29, 2025

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
@BrewTestBot BrewTestBot mentioned this pull request Jan 30, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ntBre ntBre ntBre approved these changes

Assignees

No one assigned

Labels

preview Related to preview mode features rule Implementing or modifying a lint rule

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Garrett-R @ntBre @AlexWaygood