extend warning if globs are used instead of regex to local hooks#2524
extend warning if globs are used instead of regex to local hooks#2524asottile merged 1 commit intopre-commit:mainfrom
Conversation
pre_commit/clientlib.py
Outdated
| OptionalSensibleRegexAtHook('files', cfgv.check_string), | ||
| OptionalSensibleRegexAtHook('exclude', cfgv.check_string), |
There was a problem hiding this comment.
we don't want to warn users about things outside their control (remote repositories doing this incorrectly)
There was a problem hiding this comment.
makes sense, thanks for the direction. I think I have a better understanding of the cfgv stuff after looking at it a bit, so I'll post a revision here in a minute.
There was a problem hiding this comment.
I reworked it per your call-out.
Also noticed that the existing validators for this were calling super().check and passing a check func of check_string, which as far as I can tell is superfluous work since there are already validators on CONFIG_HOOK_DICT and CONFIG_SCHEMA for files & exclude which use check_string_regex. So, I removed the super().check calls and just passed check_any instead, similar to how you did the NotAllowed validator. Let me know if that doesn't jive, though.
Thanks!
There was a problem hiding this comment.
compare:
x = OptionalSensibleRegexAtHook('files', cfgv.check_string)
x.check({'files': None})with super:
$ python3 t.py
Traceback (most recent call last):
File "/tmp/add-trailing-comma/t.py", line 25, in <module>
x.check({'files': None})
File "/tmp/add-trailing-comma/t.py", line 7, in check
super().check(dct)
File "/tmp/add-trailing-comma/venv/lib/python3.10/site-packages/cfgv.py", line 53, in _check_optional
with validate_context(f'At key: {self.key}'):
File "/usr/lib/python3.10/contextlib.py", line 153, in __exit__
self.gen.throw(typ, value, traceback)
File "/tmp/add-trailing-comma/venv/lib/python3.10/site-packages/cfgv.py", line 34, in validate_context
raise ValidationError(e, ctx=msg).with_traceback(tb) from None
File "/tmp/add-trailing-comma/venv/lib/python3.10/site-packages/cfgv.py", line 31, in validate_context
yield
File "/tmp/add-trailing-comma/venv/lib/python3.10/site-packages/cfgv.py", line 54, in _check_optional
self.check_fn(dct[self.key])
File "/tmp/add-trailing-comma/venv/lib/python3.10/site-packages/cfgv.py", line 325, in check_type_fn
raise ValidationError(
cfgv.ValidationError:
==> At key: files
=====> Expected string got NoneTypewithout super:
$ python3 t.py
Traceback (most recent call last):
File "/tmp/add-trailing-comma/t.py", line 25, in <module>
x.check({'files': None})
File "/tmp/add-trailing-comma/t.py", line 9, in check
if '/*' in dct.get(self.key, ''):
TypeError: argument of type 'NoneType' is not iterableThere was a problem hiding this comment.
roger that - made the update
There was a problem hiding this comment.
I added a test for the counterexample. Definitely need to make sure I can commit the same amount of attention I would to closed source in poking around / exploring edge cases / understanding the code base / writing tests / etc., rather than letting the novelty & excitement of open source tempt me to just grab a ticket and push something quickly. So, I apologize & appreciate the patience here - I'll do better!
f6acd19 to
8315892
Compare
8315892 to
a95f488
Compare
Resolves #2521
Apologies for the diff - those two classes were just moved up w/o modification