[ruff] Stabilize pytest-raises-ambiguous-pattern (RUF043)
#16657
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary -- Stabilizes RUF043 and moves its test to the right place. The docs look good. Test Plan -- 0 closed or open issues. 1 follow-up PR to fix typos in the test documentation and 1 bug fix to treat `)` as a regex meta-character on 2025-01-01. This can be a pretty noisy rule (+503 violations in the initial PR), so we'll see how the ecosystem check looks now.
CodSpeed Performance ReportMerging #16657 will degrade performances by 10.81%Comparing Summary
Benchmarks breakdown
|
Contributor
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF043 | 438 | 438 | 0 | 0 | 0 |
Linter (preview)
✅ ecosystem check detected no linter changes.
Contributor
Author
|
These all look like true positives and almost all with I'm not totally sure if the number of hits actually decreased or if the truncation is just different. airflow is down 7 violations but ibis and pandas are +1 each. |
Member
Do you mean compared to the initial PR? It's possible that some repositories use preview mode and have fixed the violations in their code. |
MichaReiser
approved these changes
Mar 12, 2025
Member
There was a problem hiding this comment.
It's certainly a more noisy rule and it's unfortunate we can't provide an auto fix (because Ruff can't know what the desired behavior is). But I think it's fine to stabilize
Unless we want to add a heuristic for the auto fix. E.g.:
- A string ending with
.is probably a sentence -> escape it - A string with a sequence of metacharacters (e.g.
.*or.+) is probably a regex -> make it raw. We can limit it to only support some specific patterns.
The first fix would have to be unsafe because it does change the program's semantics. The second one could be safe but I think it's still best to make user explicitly opt-in. We could document the heuristics in the rule and having a fix could reduce the churn for users.
I'd postpone the promotion to stable if the suggested fix does sound reasonable (CC: @AlexWaygood)
Contributor
Author
|
I'm going to close this for now and open an issue with Micha's fix idea. The rule is a bit newer than most of the other stabilizations anyway. I'm not opposed to reopening and merging it for v0.10, however. Just don't let me forget to add it back to the blog post! |
ntBre
added a commit
that referenced
this pull request
Sep 4, 2025
This one has been a bit contentious in the past. It usually uncovers ~700 ecosystem hits. See: - #16657 - #16690 But I think there's consensus that it's okay to merge as-is. We'd love an autofix since it's so common, but we can't reliably tell what a user meant. The pattern is ambiguous after all 😆 This is the first rule that actually needed its test case relocated, but the docs looked good.
ntBre
added a commit
that referenced
this pull request
Sep 5, 2025
This one has been a bit contentious in the past. It usually uncovers ~700 ecosystem hits. See: - #16657 - #16690 But I think there's consensus that it's okay to merge as-is. We'd love an autofix since it's so common, but we can't reliably tell what a user meant. The pattern is ambiguous after all 😆 This is the first rule that actually needed its test case relocated, but the docs looked good.
ntBre
added a commit
that referenced
this pull request
Sep 8, 2025
This one has been a bit contentious in the past. It usually uncovers ~700 ecosystem hits. See: - #16657 - #16690 But I think there's consensus that it's okay to merge as-is. We'd love an autofix since it's so common, but we can't reliably tell what a user meant. The pattern is ambiguous after all 😆 This is the first rule that actually needed its test case relocated, but the docs looked good.
ntBre
added a commit
that referenced
this pull request
Sep 10, 2025
This one has been a bit contentious in the past. It usually uncovers ~700 ecosystem hits. See: - #16657 - #16690 But I think there's consensus that it's okay to merge as-is. We'd love an autofix since it's so common, but we can't reliably tell what a user meant. The pattern is ambiguous after all 😆 This is the first rule that actually needed its test case relocated, but the docs looked good.
ntBre
added a commit
that referenced
this pull request
Sep 10, 2025
This one has been a bit contentious in the past. It usually uncovers ~700 ecosystem hits. See: - #16657 - #16690 But I think there's consensus that it's okay to merge as-is. We'd love an autofix since it's so common, but we can't reliably tell what a user meant. The pattern is ambiguous after all 😆 This is the first rule that actually needed its test case relocated, but the docs looked good.
ntBre
added a commit
that referenced
this pull request
Sep 10, 2025
This one has been a bit contentious in the past. It usually uncovers ~700 ecosystem hits. See: - #16657 - #16690 But I think there's consensus that it's okay to merge as-is. We'd love an autofix since it's so common, but we can't reliably tell what a user meant. The pattern is ambiguous after all 😆 This is the first rule that actually needed its test case relocated, but the docs looked good.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Stabilizes RUF043 and moves its test to the right place. The docs look good.
Test Plan
0 closed or open issues. 1 follow-up PR to fix typos in the test documentation and 1 bug fix to treat
)as a regex meta-character on 2025-01-07.This can be a pretty noisy rule (+503 violations in the initial PR), so we'll see how the ecosystem check looks now. I'm happy to hold off stabilizing if we think it's too noisy, just wanted to open the PR just in case.