Skip to content
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

extend warning if globs are used instead of regex to local hooks #2524

Merged
merged 1 commit into from
Sep 26, 2022

Conversation

chrisRedwine
Copy link
Contributor

Resolves #2521
Apologies for the diff - those two classes were just moved up w/o modification

Comment on lines 121 to 122
OptionalSensibleRegexAtHook('files', cfgv.check_string),
OptionalSensibleRegexAtHook('exclude', cfgv.check_string),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't want to warn users about things outside their control (remote repositories doing this incorrectly)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 NoneType

without 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 iterable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roger that - made the update

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@chrisRedwine chrisRedwine force-pushed the fix-local-regex-warnings branch 3 times, most recently from f6acd19 to 8315892 Compare September 25, 2022 16:42
Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asottile asottile merged commit 102fbb2 into pre-commit:main Sep 26, 2022
@chrisRedwine chrisRedwine deleted the fix-local-regex-warnings branch September 27, 2022 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

warnings for regexes don't apply to repo: local
2 participants