From b096c0b8f2074ec5c7e05528b27be4a1bf3df8d7 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Fri, 14 Dec 2018 12:29:52 +0000 Subject: [PATCH 1/6] Allow aliasing a hook and calling it by it's alias --- pre_commit/clientlib.py | 18 +++++++++++++++++- pre_commit/commands/run.py | 8 ++++++-- tests/commands/run_test.py | 26 ++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/pre_commit/clientlib.py b/pre_commit/clientlib.py index 2fa7b153..0722f5e6 100644 --- a/pre_commit/clientlib.py +++ b/pre_commit/clientlib.py @@ -29,6 +29,20 @@ def _make_argparser(filenames_help): return parser +class OptionalAlias(object): + + def check(self, dct): + if 'alias' in dct: + cfgv.check_string(dct['alias']) + + def apply_default(self, dct): + if 'alias' not in dct: + dct['alias'] = dct['id'] + + def remove_default(self, dct): + pass + + MANIFEST_HOOK_DICT = cfgv.Map( 'Hook', 'id', @@ -36,6 +50,7 @@ MANIFEST_HOOK_DICT = cfgv.Map( cfgv.Required('name', cfgv.check_string), cfgv.Required('entry', cfgv.check_string), cfgv.Required('language', cfgv.check_one_of(all_languages)), + cfgv.OptionalNoDefault('alias', cfgv.check_string), cfgv.Optional( 'files', cfgv.check_and(cfgv.check_string, cfgv.check_regex), '', @@ -125,6 +140,7 @@ CONFIG_HOOK_DICT = cfgv.Map( 'Hook', 'id', cfgv.Required('id', cfgv.check_string), + OptionalAlias(), # All keys in manifest hook dict are valid in a config hook dict, but # are optional. @@ -133,7 +149,7 @@ CONFIG_HOOK_DICT = cfgv.Map( *[ cfgv.OptionalNoDefault(item.key, item.check_fn) for item in MANIFEST_HOOK_DICT.items - if item.key != 'id' + if item.key not in ('id', 'alias') ] ) CONFIG_REPO_DICT = cfgv.Map( diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py index f2ff7b38..9cd3dfcf 100644 --- a/pre_commit/commands/run.py +++ b/pre_commit/commands/run.py @@ -257,13 +257,17 @@ def run(config_file, store, args, environ=os.environ): for repo in repositories(config, store): for _, hook in repo.hooks: if ( - (not args.hook or hook['id'] == args.hook) and + (not args.hook or hook['id'] == args.hook or ( + hook['alias'] and hook['alias'] == args.hook + )) and (not hook['stages'] or args.hook_stage in hook['stages']) ): repo_hooks.append((repo, hook)) if args.hook and not repo_hooks: - output.write_line('No hook with id `{}`'.format(args.hook)) + output.write_line( + 'No hook with id or alias `{}`'.format(args.hook), + ) return 1 for repo in {repo for repo, _ in repo_hooks}: diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index bb233f28..1cec51f2 100644 --- a/tests/commands/run_test.py +++ b/tests/commands/run_test.py @@ -416,6 +416,32 @@ def test_multiple_hooks_same_id(cap_out, store, repo_with_passing_hook): assert output.count(b'Bash hook') == 2 +def test_aliased_hook_run(cap_out, store, repo_with_passing_hook): + with cwd(repo_with_passing_hook): + # Add bash hook on there again, aliased + with modify_config() as config: + config['repos'][0]['hooks'].append( + {'id': 'bash_hook', 'alias': 'foo_bash'}, + ) + stage_a_file() + + ret, output = _do_run( + cap_out, store, repo_with_passing_hook, + run_opts(verbose=True, hook='bash_hook'), + ) + assert ret == 0 + # Both hooks will run since they share the same ID + assert output.count(b'Bash hook') == 2 + + ret, output = _do_run( + cap_out, store, repo_with_passing_hook, + run_opts(verbose=True, hook='foo_bash'), + ) + assert ret == 0 + # Only the aliased hook runs + assert output.count(b'Bash hook') == 1 + + def test_non_ascii_hook_id(repo_with_passing_hook, tempdir_factory): with cwd(repo_with_passing_hook): _, stdout, _ = cmd_output_mocked_pre_commit_home( From afbc57f2ad135c54677347142462075df379238b Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Mon, 17 Dec 2018 12:05:55 +0000 Subject: [PATCH 2/6] Go back to optional. Requires less changes to existing code. --- pre_commit/clientlib.py | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/pre_commit/clientlib.py b/pre_commit/clientlib.py index 0722f5e6..44599ea6 100644 --- a/pre_commit/clientlib.py +++ b/pre_commit/clientlib.py @@ -29,20 +29,6 @@ def _make_argparser(filenames_help): return parser -class OptionalAlias(object): - - def check(self, dct): - if 'alias' in dct: - cfgv.check_string(dct['alias']) - - def apply_default(self, dct): - if 'alias' not in dct: - dct['alias'] = dct['id'] - - def remove_default(self, dct): - pass - - MANIFEST_HOOK_DICT = cfgv.Map( 'Hook', 'id', @@ -50,7 +36,7 @@ MANIFEST_HOOK_DICT = cfgv.Map( cfgv.Required('name', cfgv.check_string), cfgv.Required('entry', cfgv.check_string), cfgv.Required('language', cfgv.check_one_of(all_languages)), - cfgv.OptionalNoDefault('alias', cfgv.check_string), + cfgv.Optional('alias', cfgv.check_string, ''), cfgv.Optional( 'files', cfgv.check_and(cfgv.check_string, cfgv.check_regex), '', @@ -140,7 +126,6 @@ CONFIG_HOOK_DICT = cfgv.Map( 'Hook', 'id', cfgv.Required('id', cfgv.check_string), - OptionalAlias(), # All keys in manifest hook dict are valid in a config hook dict, but # are optional. @@ -149,7 +134,7 @@ CONFIG_HOOK_DICT = cfgv.Map( *[ cfgv.OptionalNoDefault(item.key, item.check_fn) for item in MANIFEST_HOOK_DICT.items - if item.key not in ('id', 'alias') + if item.key != 'id' ] ) CONFIG_REPO_DICT = cfgv.Map( From 5840f880a92135599d868098645eb2aa7e3930de Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Wed, 26 Dec 2018 08:56:30 +0000 Subject: [PATCH 3/6] Address review comments and test failures --- pre_commit/commands/run.py | 17 ++++++++++------- tests/repository_test.py | 1 + 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py index 9cd3dfcf..713603b3 100644 --- a/pre_commit/commands/run.py +++ b/pre_commit/commands/run.py @@ -257,17 +257,20 @@ def run(config_file, store, args, environ=os.environ): for repo in repositories(config, store): for _, hook in repo.hooks: if ( - (not args.hook or hook['id'] == args.hook or ( - hook['alias'] and hook['alias'] == args.hook - )) and - (not hook['stages'] or args.hook_stage in hook['stages']) + ( + not args.hook or + hook['id'] == args.hook or + hook['alias'] == args.hook + ) and + ( + not hook['stages'] or + args.hook_stage in hook['stages'] + ) ): repo_hooks.append((repo, hook)) if args.hook and not repo_hooks: - output.write_line( - 'No hook with id or alias `{}`'.format(args.hook), - ) + output.write_line('No hook with id `{}`'.format(args.hook)) return 1 for repo in {repo for repo, _ in repo_hooks}: diff --git a/tests/repository_test.py b/tests/repository_test.py index f1b0f6e0..4d851f59 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -831,6 +831,7 @@ def test_manifest_hooks(tempdir_factory, store): 'exclude': '^$', 'files': '', 'id': 'bash_hook', + 'alias': '', 'language': 'script', 'language_version': 'default', 'log_file': '', From 79c8b1fceb4ddf6f396f7d46ac1168faee2ffb6e Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Wed, 26 Dec 2018 09:05:37 +0000 Subject: [PATCH 4/6] Allow hook alias to be used in `SKIP`. Includes test. --- pre_commit/commands/run.py | 9 +++++++++ tests/commands/run_test.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py index 713603b3..2fb107b7 100644 --- a/pre_commit/commands/run.py +++ b/pre_commit/commands/run.py @@ -86,6 +86,15 @@ def _run_single_hook(filenames, hook, repo, args, skips, cols): cols=cols, )) return 0 + elif hook['alias'] and hook['alias'] in skips: + output.write(get_hook_message( + _hook_msg_start(hook, args.verbose), + end_msg=SKIPPED, + end_color=color.YELLOW, + use_color=args.color, + cols=cols, + )) + return 0 elif not filenames and not hook['always_run']: output.write(get_hook_message( _hook_msg_start(hook, args.verbose), diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index 1cec51f2..c3d1ec5d 100644 --- a/tests/commands/run_test.py +++ b/tests/commands/run_test.py @@ -388,6 +388,38 @@ def test_skip_hook(cap_out, store, repo_with_passing_hook): assert msg in printed +def test_skip_aliased_hook(cap_out, store, repo_with_passing_hook): + with cwd(repo_with_passing_hook): + # Add bash hook on there again, aliased + with modify_config() as config: + config['repos'][0]['hooks'].append( + {'id': 'bash_hook', 'alias': 'foo_bash'}, + ) + stage_a_file() + + ret, printed = _do_run( + cap_out, store, repo_with_passing_hook, + run_opts(hook='bash_hook'), + {'SKIP': 'bash_hook'}, + ) + assert ret == 0 + # Both hooks will run since they share the same ID + assert printed.count(b'Bash hook') == 2 + for msg in (b'Bash hook', b'Skipped'): + assert msg in printed + + ret, printed = _do_run( + cap_out, store, repo_with_passing_hook, + run_opts(hook='foo_bash'), + {'SKIP': 'foo_bash'}, + ) + assert ret == 0 + # Only the aliased hook runs + assert printed.count(b'Bash hook') == 1 + for msg in (b'Bash hook', b'Skipped'): + assert msg in printed, printed + + def test_hook_id_not_in_non_verbose_output( cap_out, store, repo_with_passing_hook, ): From 8ffd1f69d7684a6303471a377df314d2308a05c9 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Thu, 27 Dec 2018 12:03:09 +0000 Subject: [PATCH 5/6] Address review comments --- pre_commit/commands/run.py | 11 +---------- tests/commands/run_test.py | 40 +++++++++++++++++--------------------- 2 files changed, 19 insertions(+), 32 deletions(-) diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py index 2fb107b7..d9280460 100644 --- a/pre_commit/commands/run.py +++ b/pre_commit/commands/run.py @@ -77,16 +77,7 @@ def _run_single_hook(filenames, hook, repo, args, skips, cols): 'replacement.'.format(hook['id'], repo.repo_config['repo']), ) - if hook['id'] in skips: - output.write(get_hook_message( - _hook_msg_start(hook, args.verbose), - end_msg=SKIPPED, - end_color=color.YELLOW, - use_color=args.color, - cols=cols, - )) - return 0 - elif hook['alias'] and hook['alias'] in skips: + if hook['id'] in skips or hook['alias'] in skips: output.write(get_hook_message( _hook_msg_start(hook, args.verbose), end_msg=SKIPPED, diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index c3d1ec5d..37e17a52 100644 --- a/tests/commands/run_test.py +++ b/tests/commands/run_test.py @@ -42,6 +42,18 @@ def repo_with_failing_hook(tempdir_factory): yield git_path +@pytest.fixture +def aliased_repo(tempdir_factory): + git_path = make_consuming_repo(tempdir_factory, 'script_hooks_repo') + with cwd(git_path): + with modify_config() as config: + config['repos'][0]['hooks'].append( + {'id': 'bash_hook', 'alias': 'foo_bash'}, + ) + stage_a_file() + yield git_path + + def stage_a_file(filename='foo.py'): open(filename, 'a').close() cmd_output('git', 'add', filename) @@ -388,17 +400,9 @@ def test_skip_hook(cap_out, store, repo_with_passing_hook): assert msg in printed -def test_skip_aliased_hook(cap_out, store, repo_with_passing_hook): - with cwd(repo_with_passing_hook): - # Add bash hook on there again, aliased - with modify_config() as config: - config['repos'][0]['hooks'].append( - {'id': 'bash_hook', 'alias': 'foo_bash'}, - ) - stage_a_file() - +def test_skip_aliased_hook(cap_out, store, aliased_repo): ret, printed = _do_run( - cap_out, store, repo_with_passing_hook, + cap_out, store, aliased_repo, run_opts(hook='bash_hook'), {'SKIP': 'bash_hook'}, ) @@ -409,7 +413,7 @@ def test_skip_aliased_hook(cap_out, store, repo_with_passing_hook): assert msg in printed ret, printed = _do_run( - cap_out, store, repo_with_passing_hook, + cap_out, store, aliased_repo, run_opts(hook='foo_bash'), {'SKIP': 'foo_bash'}, ) @@ -448,17 +452,9 @@ def test_multiple_hooks_same_id(cap_out, store, repo_with_passing_hook): assert output.count(b'Bash hook') == 2 -def test_aliased_hook_run(cap_out, store, repo_with_passing_hook): - with cwd(repo_with_passing_hook): - # Add bash hook on there again, aliased - with modify_config() as config: - config['repos'][0]['hooks'].append( - {'id': 'bash_hook', 'alias': 'foo_bash'}, - ) - stage_a_file() - +def test_aliased_hook_run(cap_out, store, aliased_repo): ret, output = _do_run( - cap_out, store, repo_with_passing_hook, + cap_out, store, aliased_repo, run_opts(verbose=True, hook='bash_hook'), ) assert ret == 0 @@ -466,7 +462,7 @@ def test_aliased_hook_run(cap_out, store, repo_with_passing_hook): assert output.count(b'Bash hook') == 2 ret, output = _do_run( - cap_out, store, repo_with_passing_hook, + cap_out, store, aliased_repo, run_opts(verbose=True, hook='foo_bash'), ) assert ret == 0 From 6d40b2a38b274e7a561322749702a00703432a66 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Thu, 27 Dec 2018 09:24:41 -0800 Subject: [PATCH 6/6] Simplify the skip test to only test skipping --- tests/commands/run_test.py | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index 37e17a52..bc891c0c 100644 --- a/tests/commands/run_test.py +++ b/tests/commands/run_test.py @@ -401,27 +401,15 @@ def test_skip_hook(cap_out, store, repo_with_passing_hook): def test_skip_aliased_hook(cap_out, store, aliased_repo): - ret, printed = _do_run( - cap_out, store, aliased_repo, - run_opts(hook='bash_hook'), - {'SKIP': 'bash_hook'}, - ) - assert ret == 0 - # Both hooks will run since they share the same ID - assert printed.count(b'Bash hook') == 2 - for msg in (b'Bash hook', b'Skipped'): - assert msg in printed - ret, printed = _do_run( cap_out, store, aliased_repo, run_opts(hook='foo_bash'), {'SKIP': 'foo_bash'}, ) assert ret == 0 - # Only the aliased hook runs - assert printed.count(b'Bash hook') == 1 + # Only the aliased hook runs and is skipped for msg in (b'Bash hook', b'Skipped'): - assert msg in printed, printed + assert printed.count(msg) == 1 def test_hook_id_not_in_non_verbose_output(