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
Add warning for regular expression with [\/] (#2043) #2053
Conversation
pre_commit/clientlib.py
Outdated
if r'[\/]' in dct.get(self.key, ''): | ||
logger.warning( | ||
f'Pre-commit normalizes the slashes in {self.key!r} field in ' | ||
f"hook {dct.get('id')!r} to forward slashes, so you can use " | ||
f'/ instead of [\\/]', | ||
) |
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 would just add these to the existing SensibleRegex checks
also this should check for both r'[\/]'
and r'[/\]'
since those are both valid spellings of "either slash"
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've merged the check into the existing functions. However, r[/\]
is not a valid regex, \]
is an escape sequence to enable things like [...\]]
.
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.
[/\\]
is though, with the same issue
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.
Ok, I'll add it. Although it seems unlikely to me that someone would use [/\\]
instead of just [\/]
.
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've kept it in three ifs. If you feel looping through the regexes in for loop would be more readable, let me know.
pre_commit/clientlib.py
Outdated
|
||
if r'[\/]' in dct.get(self.key, ''): | ||
logger.warning( | ||
f'Pre-commit normalizes the slashes in {self.key!r} field in ' |
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.
pre-commit is not capitalized
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 capitalized it as a sentence, even though it wasn't one. Fixed.
pre_commit/clientlib.py
Outdated
logger.warning( | ||
f'Pre-commit normalizes the slashes in the top-level ' | ||
f'{self.key!r} field to forward slashes, so you can use / ' | ||
f'instead of [\\/]', |
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.
you can use fr'...'
for a raw f-string by the way
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.
Fixed.
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.
@asottile @radek-sprta Hi! Thanks for this feature. I just wanted to point out that there's one more permutation that's currently not addressed and you migth want to consider, |
@sanjioh can you make a new issue please? |
Add warning about unnecessary slash regex as per #2043