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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Types #551

Merged
merged 4 commits into from Jul 3, 2017
Jump to file or symbol
Failed to load files and symbols.
+36 鈭0
Diff settings

Always

Just for now

Viewing a subset of changes. View all

Implement types filtering

  • Loading branch information...
asottile committed Jul 2, 2017
commit a58d99ac409183b0e8eeb2b41f34f2c8e9bf4dc0
View
@@ -6,18 +6,24 @@
import subprocess
import sys
from identify.identify import tags_from_path
from pre_commit import color
from pre_commit import git
from pre_commit import output
from pre_commit.output import get_hook_message
from pre_commit.staged_files_only import staged_files_only
from pre_commit.util import cmd_output
from pre_commit.util import memoize_by_cwd
from pre_commit.util import noop_context
logger = logging.getLogger('pre_commit')
tags_from_path = memoize_by_cwd(tags_from_path)
def _get_skips(environ):
skips = environ.get('SKIP', '')
return {skip.strip() for skip in skips.split(',') if skip.strip()}
@@ -37,6 +43,14 @@ def get_changed_files(new, old):
)[1].splitlines()
def filter_filenames_by_types(filenames, types):
types = frozenset(types)
return tuple(
filename for filename in filenames
if tags_from_path(filename) >= types

This comment has been minimized.

@Lucas-C

Lucas-C Jul 2, 2017

Contributor

Nice ! 馃憤

@Lucas-C

Lucas-C Jul 2, 2017

Contributor

Nice ! 馃憤

This comment has been minimized.

@chriskuehl

chriskuehl Jul 2, 2017

Member

Do you think it makes sense for the list of tags to be an OR rather than an AND?

I'm wondering if there's a case where someone will do something like types: [python, ruby] and be surprised that it matches nothing rather than all python and ruby files.

I'm having trouble coming up with a scenario where I'd want to target only files which have multiple tags... but I also can't really think of too many cases where I'd want to target both python and ruby but not just text, so maybe both cases are pretty rare?

My original (much more complicated) branch just checked for any intersection of the tags, but I didn't really spend much time considering the alternative.

@chriskuehl

chriskuehl Jul 2, 2017

Member

Do you think it makes sense for the list of tags to be an OR rather than an AND?

I'm wondering if there's a case where someone will do something like types: [python, ruby] and be surprised that it matches nothing rather than all python and ruby files.

I'm having trouble coming up with a scenario where I'd want to target only files which have multiple tags... but I also can't really think of too many cases where I'd want to target both python and ruby but not just text, so maybe both cases are pretty rare?

My original (much more complicated) branch just checked for any intersection of the tags, but I didn't really spend much time considering the alternative.

This comment has been minimized.

@asottile

asottile Jul 2, 2017

Member

Yeah I do think it should be AND - I also think I should implement exclude_types as we have (files, exclude) but no way to do that with types.

@asottile

asottile Jul 2, 2017

Member

Yeah I do think it should be AND - I also think I should implement exclude_types as we have (files, exclude) but no way to do that with types.

This comment has been minimized.

@asottile

asottile Jul 2, 2017

Member

Actually, maybe it should be OR - though I can't think of a compelling reason why someone would ever use more than one tag.

@asottile

asottile Jul 2, 2017

Member

Actually, maybe it should be OR - though I can't think of a compelling reason why someone would ever use more than one tag.

This comment has been minimized.

@chriskuehl

chriskuehl Jul 2, 2017

Member

Yeah, I can't really come up with a real use-case for either. It's still possible to get the OR behavior by specifying the hook twice if you really need it. I'm fine with either way.

fwiw all of the standard hooks were easy to write with just one type: https://github.com/chriskuehl/pre-commit-hooks/blob/new-typers/hooks.yaml

@chriskuehl

chriskuehl Jul 2, 2017

Member

Yeah, I can't really come up with a real use-case for either. It's still possible to get the OR behavior by specifying the hook twice if you really need it. I'm fine with either way.

fwiw all of the standard hooks were easy to write with just one type: https://github.com/chriskuehl/pre-commit-hooks/blob/new-typers/hooks.yaml

This comment has been minimized.

@asottile

asottile Jul 3, 2017

Member

shrugs, easier to leave it how it is so I'll do that. If someone has a legitimate usecase for OR that neither exclude_types nor separate hooks satisfies we can revisit this.

@asottile

asottile Jul 3, 2017

Member

shrugs, easier to leave it how it is so I'll do that. If someone has a legitimate usecase for OR that neither exclude_types nor separate hooks satisfies we can revisit this.

)
def get_filenames(args, include_expr, exclude_expr):
if args.origin and args.source:
getter = git.get_files_matching(
@@ -59,6 +73,7 @@ def get_filenames(args, include_expr, exclude_expr):
def _run_single_hook(hook, repo, args, skips, cols):
filenames = get_filenames(args, hook['files'], hook['exclude'])
filenames = filter_filenames_by_types(filenames, hook['types'])
if hook['id'] in skips:
output.write(get_hook_message(
_hook_msg_start(hook, args.verbose),
@@ -0,0 +1,5 @@
- id: python-files
name: Python files
entry: bin/hook.sh
language: script
types: [python]
@@ -0,0 +1,3 @@
#!/usr/bin/env bash
echo $@
exit 1
View
@@ -153,6 +153,19 @@ def test_hook_that_modifies_but_returns_zero(
)
def test_types_hook_repository(
cap_out, tempdir_factory, mock_out_store_directory,
):
git_path = make_consuming_repo(tempdir_factory, 'types_repo')
with cwd(git_path):
stage_a_file('bar.py')
stage_a_file('bar.notpy')
ret, printed = _do_run(cap_out, git_path, _get_opts())
assert ret == 1
assert b'bar.py' in printed
assert b'bar.notpy' not in printed

This comment has been minimized.

@Lucas-C

Lucas-C Jul 2, 2017

Contributor

The new types_repo/bin/hook.sh file is not tested ?

@Lucas-C

Lucas-C Jul 2, 2017

Contributor

The new types_repo/bin/hook.sh file is not tested ?

This comment has been minimized.

@asottile

asottile Jul 2, 2017

Member

All hook.sh does is print the filenames and invariantly fail -- I think this tests that?

@asottile

asottile Jul 2, 2017

Member

All hook.sh does is print the filenames and invariantly fail -- I think this tests that?

This comment has been minimized.

@Lucas-C

Lucas-C Jul 2, 2017

Contributor

Yes sure. Actually I thought hook.sh role was to test shebang detection.
My mistake

@Lucas-C

Lucas-C Jul 2, 2017

Contributor

Yes sure. Actually I thought hook.sh role was to test shebang detection.
My mistake

def test_show_diff_on_failure(
capfd, cap_out, tempdir_factory, mock_out_store_directory,
):
ProTip! Use n and p to navigate between commits in a pull request.