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

Add types_or #1677

Merged
merged 1 commit into from Nov 3, 2020
Merged

Add types_or #1677

merged 1 commit into from Nov 3, 2020

Conversation

MarcoGorelli
Copy link
Contributor

closes #607

@MarcoGorelli MarcoGorelli marked this pull request as draft November 2, 2020 16:32
@MarcoGorelli MarcoGorelli force-pushed the types_or branch 2 times, most recently from e5b7b12 to b328a3e Compare November 2, 2020 16:37
@MarcoGorelli MarcoGorelli marked this pull request as ready for review November 2, 2020 16:38
@MarcoGorelli MarcoGorelli marked this pull request as draft November 2, 2020 16:59
@MarcoGorelli MarcoGorelli marked this pull request as ready for review November 2, 2020 17:08
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.

good start!

I believe check_hooks_apply and check_useless_excludes need to be updated

frozenset(types),
frozenset(types_or),
frozenset(exclude_types),
)
Copy link
Member

Choose a reason for hiding this comment

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

this should be three separate lines

ret = []
for filename in names:
tags = self._types_for_file(filename)
if tags >= types and not tags & exclude_types:
if tags >= types and not tags & exclude_types and tags & types_or:
Copy link
Member

Choose a reason for hiding this comment

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

teeny tiny thing, I'd order the conditions the same as the parameters

hook['types'],
hook['types_or'],
hook['exclude_types'],
)
Copy link
Member

Choose a reason for hiding this comment

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

""

entry: bin/hook.sh
language: script
types: [file]
types_or: [python, cython]
Copy link
Member

Choose a reason for hiding this comment

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

these are usually easier to demo with a local repo

I'd probably not write a full integration test for this feature as well but it looks like there maybe aren't great examples using the matcher

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback, but I'm not sure I understand - are there any existing tests I should look at as examples? If not, I'll try to figure this out, no worries

@@ -0,0 +1,3 @@
#!/usr/bin/env bash
echo $@
Copy link
Member

Choose a reason for hiding this comment

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

~technically this should be echo "$@"

@MarcoGorelli
Copy link
Contributor Author

good start!

Thanks!

I believe check_hooks_apply and check_useless_excludes need to be updated

Sorry, this isn't clear to me - I updated check_useless_excludes, and it's not obvious to me how check_hooks_apply needs to be updated, any hints?

@asottile
Copy link
Member

asottile commented Nov 2, 2020

I believe check_hooks_apply and check_useless_excludes need to be updated

Sorry, this isn't clear to me - I updated check_useless_excludes, and it's not obvious to me how check_hooks_apply needs to be updated, any hints?

oh maybe I'm wrong, maybe it already works

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.

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.

OR relationship with types
2 participants