fix: Add missing warning for regular expression with [\\/]#2154
fix: Add missing warning for regular expression with [\\/]#2154asottile merged 2 commits intopre-commit:masterfrom
Conversation
test: Test case parameters for said regular expression refactor: For-loop for regex warnings instead of multiple if statements resolves pre-commit#2151
pre_commit/clientlib.py
Outdated
| fr'in hook {dct.get("id")!r} to forward slashes, so you ' | ||
| fr'can use / instead of [/\\]', | ||
| ) | ||
| for fwd_slash_re in [r'[\\/]', r'[\/]', r'[/\\]']: |
There was a problem hiding this comment.
this should be a tuple
while the code was originally written as separate if statements to make sure everything is covered, this is fine I guess
There was a problem hiding this comment.
Fixed the first, and making excuses for the second:
Figured I'd remove some duplication, but if requested, I will revert it back to if statements.
pre_commit/clientlib.py
Outdated
| fr'in hook {dct.get("id")!r} to forward slashes, so you ' | ||
| fr'can use / instead of [/\\]', | ||
| ) | ||
| for fwd_slash_re in [r'[\\/]', r'[\/]', r'[/\\]']: |
There was a problem hiding this comment.
Also: should r'[\/]' trigger the warning at all? The matched character is a forward slash, mentioning "normalization to forward slashes" seems odd in this case.
There was a problem hiding this comment.
This is something I expected you to comment on, and I'm glad for that.
Since this was accepted in the previous pull request regarding these regexes, without any comments on the subject, I felt like confirming if this is meant to be a style guideline?
In this case, the warning would (sort of) make sense.
There was a problem hiding this comment.
Hello, and thanks for working on this.
Yes, you’re right that pre-commit could warn about r'[\/]' as a suggestion about style (I assume the issue didn’t come up in the original PR for an oversight).
But in that case, I’d be more inclined to extract the check into a separate one, with its own proper message.
I understand that this could be quite a nit pick, and that grouping all the regexes under the same message would be equally acceptable, although a little less precise with regards to semantics.
@asottile any thoughts about this?
There was a problem hiding this comment.
yeah so admittedly the original task was a mistake on my part since r'[\/]' is the same as r'[/]' -- but since there's already several levels of where escaping is questionable (yaml rules, then python rules, then regex rules) I think it's still correct to warn about that
the flip r'[/\]' doesn't need a warning because it's an invalid regex (unclosed ]) so something will already let the user know something is broken
test: Test case parameters for said regular expression
refactor: For-loop for regex warnings instead of multiple if statements
resolves #2151