Ignore quantifiers when splitting comma-separated regexes #7241
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.
| regexps: deque[deque[str] | None] = deque([None]) | ||
| open_braces = False | ||
| for char in value: | ||
| if char == "{": |
There was a problem hiding this comment.
Depending on the result of the discussion we should warn the user of deprecation here.
There was a problem hiding this comment.
wouldn't it be on line 346? That's where we split regular expressions.
There was a problem hiding this comment.
I know a year ago we were floating the idea of deprecations, but we're not ready to commit to a new plan IMO.
Pierre-Sassoulas
added
the
Needs decision 🔒
Pull Request Test Coverage Report for Build 2762066289
💛 - Coveralls |
Overkill, slows down unit tests Co-authored-by: Pierre Sassoulas <[email protected]>
|
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit b48c339 |
jacobtylerwalls
added
Needs review 🔍
| @@ -0,0 +1,5 @@ | |||
| When parsing comma-separated lists of regular expressions in the config, ignore | |||
| commas that are inside braces since those indicate quantiers, not dilineation | |||
There was a problem hiding this comment.
| commas that are inside braces since those indicate quantiers, not dilineation | |
| commas that are inside braces since those indicate quantifiers, not delineation |
| @@ -5,6 +5,8 @@ | |||
| from __future__ import annotations | |||
|
|
|||
| import os | |||
| import re | |||
| import timeit | |||
There was a problem hiding this comment.
| import timeit |
| assert _template_run(in_string) == [re.compile(regex) for regex in expected] | ||
|
|
||
|
|
||
|
|
jacobtylerwalls
removed
the
Needs review 🔍
|
@lbenezriravin If you enable maintainers to push to this fork I can resolve the merge conflicts for you and add the cosmetic changes. |
| @@ -34,6 +35,7 @@ | |||
| "HAS_ISORT_5", | |||
| "IsortDriver", | |||
| "_check_csv", | |||
| "_check_regexp_csv", | |||
There was a problem hiding this comment.
Since this is private, this should not be in __all__.
| "_check_regexp_csv", |
There was a problem hiding this comment.
The PR currently uses it on the module, so it has to stay, unless we refactor. It was a reasonable choice for the PR author to adhere to the existing muddled pattern. I'm inclined to merge as is.
Pierre-Sassoulas
added
the
Waiting on author
|
Will merge in #8898. Thanks! |
Type of Changes
Description
When parsing comma-separated lists of regular expressions in the config, ignore
commas that are inside braces since those indicate quantifiers, not delineation
between expressions.
There may be conflicts with #7228, lmk if you'd prefer if this gets merged first or second.
Closes #7229