-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Ignore quantifiers when splitting comma-separated regexes #8898
Conversation
Do not split on commas if they are between braces, since that indicates a quantifier. Also added a protection for slow implementations since existing workarounds may result in long strings of chained regular expressions.
Overkill, slows down unit tests Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
This comment has been minimized.
This comment has been minimized.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8898 +/- ##
==========================================
- Coverage 95.88% 95.88% -0.01%
==========================================
Files 173 173
Lines 18552 18570 +18
==========================================
+ Hits 17789 17806 +17
- Misses 763 764 +1
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'm going to revert the unrelated doc/news updates from fdd914a, because that will stop this from backporting cleanly. |
This reverts commit fdd914a.
r"""Split a comma-separated list of regexps, taking care to avoid splitting | ||
a regex employing a comma as quantifier, as in `\d{1,2}`.""" | ||
if isinstance(value, (list, tuple)): | ||
yield from value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov message looks important since that line yields a value and apparently isn’t covered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No current code path sends a list of strings to this util, or the one above it (also missing coverage for the analogous line). Should we just delete these lines? Not certain--I think it makes sense to let this util be this permissive. Also helps guard against unexpected input. I'm inclined to leave as is. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think taking the time to try to understand is not worth it vs just not touching it.
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit 4de6f9d |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-maintenance/2.17.x maintenance/2.17.x
# Navigate to the new working tree
cd .worktrees/backport-maintenance/2.17.x
# Create a new branch
git switch --create backport-8898-to-maintenance/2.17.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d28def9f9eb5e55d3571b53b11f42e4142d53167
# Push it to GitHub
git push --set-upstream origin backport-8898-to-maintenance/2.17.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-maintenance/2.17.x Then, create a pull request where the |
…#8898) Do not split on commas if they are between braces, since that indicates a quantifier. Also added a protection for slow implementations since existing workarounds may result in long strings of chained regular expressions. Adjust existing test for invalid regex to be truly invalid Co-authored-by: lihu <lihu.ben-ezri-ravin@portalinstruments.com> (cherry picked from commit d28def9)
) Do not split on commas if they are between braces, since that indicates a quantifier. Also added a protection for slow implementations since existing workarounds may result in long strings of chained regular expressions. Adjust existing test for invalid regex to be truly invalid Co-authored-by: lihu <lihu.ben-ezri-ravin@portalinstruments.com> (cherry picked from commit d28def9)
…#8898) Do not split on commas if they are between braces, since that indicates a quantifier. Also added a protection for slow implementations since existing workarounds may result in long strings of chained regular expressions. Adjust existing test for invalid regex to be truly invalid Co-authored-by: lihu <lihu.ben-ezri-ravin@portalinstruments.com>
Successor to #7241 with cosmetic updates applied and merge conflicts/tests fixed.