From 88c676a7c18d9ebe4dbdbd8d1851ba2c98b10dae Mon Sep 17 00:00:00 2001 From: Paul Hooijenga Date: Sun, 22 Oct 2017 16:40:19 +0200 Subject: [PATCH 1/6] Add support for meta hooks --- pre_commit/clientlib.py | 9 +++++++- pre_commit/repository.py | 43 ++++++++++++++++++++++++++++++++++++++ pre_commit/schema.py | 11 ++++++++++ tests/commands/run_test.py | 25 ++++++++++++++++++++++ tests/repository_test.py | 12 +++++++++++ tests/schema_test.py | 31 +++++++++++++++++++++++++++ 6 files changed, 130 insertions(+), 1 deletion(-) diff --git a/pre_commit/clientlib.py b/pre_commit/clientlib.py index 11750b74..3c086cb9 100644 --- a/pre_commit/clientlib.py +++ b/pre_commit/clientlib.py @@ -98,6 +98,8 @@ def validate_manifest_main(argv=None): _LOCAL_SENTINEL = 'local' +_META_SENTINEL = 'meta' + CONFIG_HOOK_DICT = schema.Map( 'Hook', 'id', @@ -121,7 +123,8 @@ CONFIG_REPO_DICT = schema.Map( schema.Conditional( 'sha', schema.check_string, - condition_key='repo', condition_value=schema.Not(_LOCAL_SENTINEL), + condition_key='repo', + condition_value=schema.NotIn((_LOCAL_SENTINEL, _META_SENTINEL)), ensure_absent=True, ), ) @@ -138,6 +141,10 @@ def is_local_repo(repo_entry): return repo_entry['repo'] == _LOCAL_SENTINEL +def is_meta_repo(repo_entry): + return repo_entry['repo'] == _META_SENTINEL + + class InvalidConfigError(FatalError): pass diff --git a/pre_commit/repository.py b/pre_commit/repository.py index 4d7f0a5a..b0858ba9 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -5,6 +5,7 @@ import json import logging import os import shutil +import sys from collections import defaultdict import pkg_resources @@ -14,6 +15,7 @@ import pre_commit.constants as C from pre_commit import five from pre_commit import git from pre_commit.clientlib import is_local_repo +from pre_commit.clientlib import is_meta_repo from pre_commit.clientlib import MANIFEST_HOOK_DICT from pre_commit.languages.all import languages from pre_commit.languages.helpers import environment_dir @@ -128,6 +130,8 @@ class Repository(object): def create(cls, config, store): if is_local_repo(config): return LocalRepository(config, store) + elif is_meta_repo(config): + return MetaRepository(config, store) else: return cls(config, store) @@ -242,6 +246,45 @@ class LocalRepository(Repository): return tuple(ret) +class MetaRepository(LocalRepository): + meta_hooks = { + 'test-hook': { + 'name': 'Test Hook', + 'files': '', + 'language': 'system', + 'entry': 'echo "Hello World!"', + 'always_run': True, + }, + } + + @cached_property + def hooks(self): + for hook in self.repo_config['hooks']: + if hook['id'] not in self.meta_hooks: + logger.error( + '`{}` is not a valid meta hook. ' + 'Typo? Perhaps it is introduced in a newer version? ' + 'Often `pre-commit autoupdate` fixes this.'.format( + hook['id'], + ), + ) + exit(1) + + return tuple( + ( + hook['id'], + apply_defaults( + validate( + dict(self.meta_hooks[hook['id']], **hook), + MANIFEST_HOOK_DICT, + ), + MANIFEST_HOOK_DICT, + ), + ) + for hook in self.repo_config['hooks'] + ) + + class _UniqueList(list): def __init__(self): self._set = set() diff --git a/pre_commit/schema.py b/pre_commit/schema.py index e20f74cc..e85c2303 100644 --- a/pre_commit/schema.py +++ b/pre_commit/schema.py @@ -101,6 +101,9 @@ def _check_conditional(self, dct): if isinstance(self.condition_value, Not): op = 'is' cond_val = self.condition_value.val + elif isinstance(self.condition_value, NotIn): + op = 'is any of' + cond_val = self.condition_value.values else: op = 'is not' cond_val = self.condition_value @@ -206,6 +209,14 @@ class Not(object): return other is not MISSING and other != self.val +class NotIn(object): + def __init__(self, values): + self.values = values + + def __eq__(self, other): + return other is not MISSING and other not in self.values + + def check_any(_): pass diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index d6812ae5..e52716fa 100644 --- a/tests/commands/run_test.py +++ b/tests/commands/run_test.py @@ -645,6 +645,31 @@ def test_local_hook_fails( ) +def test_meta_hook_passes( + cap_out, repo_with_passing_hook, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'test-hook'), + )), + ), + ), + )) + add_config_to_repo(repo_with_passing_hook, config) + + _test_run( + cap_out, + repo_with_passing_hook, + opts={'verbose': True}, + expected_outputs=[b'Hello World!'], + expected_ret=0, + stage=False, + ) + + @pytest.yield_fixture def modified_config_repo(repo_with_passing_hook): with modify_config(repo_with_passing_hook, commit=False) as config: diff --git a/tests/repository_test.py b/tests/repository_test.py index 37a609ba..263ce1ea 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -709,6 +709,18 @@ def test_hook_id_not_present(tempdir_factory, store, fake_log_handler): ) +def test_meta_hook_not_present(store, fake_log_handler): + config = {'repo': 'meta', 'hooks': [{'id': 'i-dont-exist'}]} + repo = Repository.create(config, store) + with pytest.raises(SystemExit): + repo.require_installed() + assert fake_log_handler.handle.call_args[0][0].msg == ( + '`i-dont-exist` is not a valid meta hook. ' + 'Typo? Perhaps it is introduced in a newer version? ' + 'Often `pre-commit autoupdate` fixes this.' + ) + + def test_too_new_version(tempdir_factory, store, fake_log_handler): path = make_repo(tempdir_factory, 'script_hooks_repo') with modify_manifest(path) as manifest: diff --git a/tests/schema_test.py b/tests/schema_test.py index c2ecf0fa..06f28e76 100644 --- a/tests/schema_test.py +++ b/tests/schema_test.py @@ -19,6 +19,7 @@ from pre_commit.schema import load_from_filename from pre_commit.schema import Map from pre_commit.schema import MISSING from pre_commit.schema import Not +from pre_commit.schema import NotIn from pre_commit.schema import Optional from pre_commit.schema import OptionalNoDefault from pre_commit.schema import remove_defaults @@ -107,6 +108,16 @@ def test_not(val, expected): assert (compared == val) is expected +@pytest.mark.parametrize( + ('values', 'expected'), + (('bar', True), ('foo', False), (MISSING, False)), +) +def test_not_in(values, expected): + compared = NotIn(('baz', 'foo')) + assert (values == compared) is expected + assert (compared == values) is expected + + trivial_array_schema = Array(Map('foo', 'id')) @@ -196,6 +207,13 @@ map_conditional_absent_not = Map( condition_key='key', condition_value=Not(True), ensure_absent=True, ), ) +map_conditional_absent_not_in = Map( + 'foo', 'key', + Conditional( + 'key2', check_bool, + condition_key='key', condition_value=NotIn((1, 2)), ensure_absent=True, + ), +) @pytest.mark.parametrize('schema', (map_conditional, map_conditional_not)) @@ -248,6 +266,19 @@ def test_ensure_absent_conditional_not(): ) +def test_ensure_absent_conditional_not_in(): + with pytest.raises(ValidationError) as excinfo: + validate({'key': 1, 'key2': True}, map_conditional_absent_not_in) + _assert_exception_trace( + excinfo.value, + ( + 'At foo(key=1)', + 'Expected key2 to be absent when key is any of (1, 2), ' + 'found key2: True', + ), + ) + + def test_no_error_conditional_absent(): validate({}, map_conditional_absent) validate({}, map_conditional_absent_not) From 8df11ee7aaa53c6055d5b22bdd8ef82afb5be6d7 Mon Sep 17 00:00:00 2001 From: Paul Hooijenga Date: Mon, 23 Oct 2017 14:29:08 +0200 Subject: [PATCH 2/6] Implement check-useless-excludes meta hook --- pre_commit/meta_hooks/__init__.py | 0 .../meta_hooks/check_useless_excludes.py | 41 +++++++++++ pre_commit/repository.py | 14 ++-- tests/commands/run_test.py | 71 ++++++++++++++++++- 4 files changed, 118 insertions(+), 8 deletions(-) create mode 100644 pre_commit/meta_hooks/__init__.py create mode 100644 pre_commit/meta_hooks/check_useless_excludes.py diff --git a/pre_commit/meta_hooks/__init__.py b/pre_commit/meta_hooks/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/pre_commit/meta_hooks/check_useless_excludes.py b/pre_commit/meta_hooks/check_useless_excludes.py new file mode 100644 index 00000000..8e891bc1 --- /dev/null +++ b/pre_commit/meta_hooks/check_useless_excludes.py @@ -0,0 +1,41 @@ +import re +import sys + +import pre_commit.constants as C +from pre_commit.clientlib import load_config +from pre_commit.git import get_all_files + + +def exclude_matches_any(filenames, include, exclude): + include_re, exclude_re = re.compile(include), re.compile(exclude) + for filename in filenames: + if include_re.search(filename) and exclude_re.search(filename): + return True + return False + + +def check_useless_excludes(config_file=None): + config = load_config(config_file or C.CONFIG_FILE) + files = get_all_files() + useless_excludes = False + + exclude = config.get('exclude') + if exclude != '^$' and not exclude_matches_any(files, '', exclude): + print('The global exclude pattern does not match any files') + useless_excludes = True + + for repo in config['repos']: + for hook in repo['hooks']: + include, exclude = hook.get('files', ''), hook.get('exclude') + if exclude and not exclude_matches_any(files, include, exclude): + print( + 'The exclude pattern for {} does not match any files' + .format(hook['id']) + ) + useless_excludes = True + + return useless_excludes + + +if __name__ == '__main__': + sys.exit(check_useless_excludes()) diff --git a/pre_commit/repository.py b/pre_commit/repository.py index b0858ba9..cb53fc85 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -4,6 +4,7 @@ import io import json import logging import os +import pipes import shutil import sys from collections import defaultdict @@ -247,13 +248,16 @@ class LocalRepository(Repository): class MetaRepository(LocalRepository): + # Note: the hook `entry` is passed through `shlex.split()` by the command + # runner, so to prevent issues with spaces and backslashes (on Windows) it + # must be quoted here. meta_hooks = { - 'test-hook': { - 'name': 'Test Hook', - 'files': '', + 'check-useless-excludes': { + 'name': 'Check for useless excludes', + 'files': '.pre-commit-config.yaml', 'language': 'system', - 'entry': 'echo "Hello World!"', - 'always_run': True, + 'entry': pipes.quote(sys.executable), + 'args': ['-m', 'pre_commit.meta_hooks.check_useless_excludes'], }, } diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index e52716fa..27fa9eea 100644 --- a/tests/commands/run_test.py +++ b/tests/commands/run_test.py @@ -653,7 +653,7 @@ def test_meta_hook_passes( ( 'hooks', ( OrderedDict(( - ('id', 'test-hook'), + ('id', 'check-useless-excludes'), )), ), ), @@ -663,13 +663,78 @@ def test_meta_hook_passes( _test_run( cap_out, repo_with_passing_hook, - opts={'verbose': True}, - expected_outputs=[b'Hello World!'], + opts={}, + expected_outputs=[b'Check for useless excludes'], expected_ret=0, stage=False, ) +def test_useless_exclude_global( + cap_out, repo_with_passing_hook, mock_out_store_directory, +): + config = OrderedDict(( + ('exclude', 'foo'), + ( + 'repos', [ + OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + )), + ), + ), + )), + ], + ), + )) + add_config_to_repo(repo_with_passing_hook, config) + + _test_run( + cap_out, + repo_with_passing_hook, + opts={'all_files': True}, + expected_outputs=[ + b'Check for useless excludes', + b'The global exclude pattern does not match any files', + ], + expected_ret=1, + stage=False, + ) + + +def test_useless_exclude_for_hook( + cap_out, repo_with_passing_hook, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + ('exclude', 'foo'), + )), + ), + ), + )) + add_config_to_repo(repo_with_passing_hook, config) + + _test_run( + cap_out, + repo_with_passing_hook, + opts={'all_files': True}, + expected_outputs=[ + b'Check for useless excludes', + b'The exclude pattern for check-useless-excludes ' + b'does not match any files', + ], + expected_ret=1, + stage=False, + ) + + @pytest.yield_fixture def modified_config_repo(repo_with_passing_hook): with modify_config(repo_with_passing_hook, commit=False) as config: From 8a0dd01c7e985970e62d3ff57841b0b0c485b8a3 Mon Sep 17 00:00:00 2001 From: Paul Hooijenga Date: Wed, 25 Oct 2017 09:35:39 +0200 Subject: [PATCH 3/6] Implement check-files-matches-any meta hook --- .../meta_hooks/check_files_matches_any.py | 36 +++++++++++++++++++ pre_commit/repository.py | 7 ++++ tests/commands/run_test.py | 33 +++++++++++++++++ 3 files changed, 76 insertions(+) create mode 100644 pre_commit/meta_hooks/check_files_matches_any.py diff --git a/pre_commit/meta_hooks/check_files_matches_any.py b/pre_commit/meta_hooks/check_files_matches_any.py new file mode 100644 index 00000000..88b4806f --- /dev/null +++ b/pre_commit/meta_hooks/check_files_matches_any.py @@ -0,0 +1,36 @@ +import re +import sys + +import pre_commit.constants as C +from pre_commit.clientlib import load_config +from pre_commit.git import get_all_files + + +def files_matches_any(filenames, include): + include_re = re.compile(include) + for filename in filenames: + if include_re.search(filename): + return True + return False + + +def check_files_matches_any(config_file=None): + config = load_config(config_file or C.CONFIG_FILE) + files = get_all_files() + files_not_matched = False + + for repo in config['repos']: + for hook in repo['hooks']: + include = hook.get('files', '') + if include and not files_matches_any(files, include): + print( + 'The files pattern for {} does not match any files' + .format(hook['id']) + ) + files_not_matched = True + + return files_not_matched + + +if __name__ == '__main__': + sys.exit(check_files_matches_any()) diff --git a/pre_commit/repository.py b/pre_commit/repository.py index cb53fc85..45389cb4 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -259,6 +259,13 @@ class MetaRepository(LocalRepository): 'entry': pipes.quote(sys.executable), 'args': ['-m', 'pre_commit.meta_hooks.check_useless_excludes'], }, + 'check-files-matches-any': { + 'name': 'Check hooks match any files', + 'files': '.pre-commit-config.yaml', + 'language': 'system', + 'entry': pipes.quote(sys.executable), + 'args': ['-m', 'pre_commit.meta_hooks.check_files_matches_any'], + }, } @cached_property diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index 27fa9eea..24771d22 100644 --- a/tests/commands/run_test.py +++ b/tests/commands/run_test.py @@ -735,6 +735,39 @@ def test_useless_exclude_for_hook( ) +def test_files_match_any( + cap_out, repo_with_passing_hook, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-files-matches-any'), + )), + OrderedDict(( + ('id', 'check-useless-excludes'), + ('files', 'foo'), + )), + ), + ), + )) + add_config_to_repo(repo_with_passing_hook, config) + + _test_run( + cap_out, + repo_with_passing_hook, + opts={'all_files': True}, + expected_outputs=[ + b'Check hooks match any files', + b'The files pattern for check-useless-excludes ' + b'does not match any files', + ], + expected_ret=1, + stage=False, + ) + + @pytest.yield_fixture def modified_config_repo(repo_with_passing_hook): with modify_config(repo_with_passing_hook, commit=False) as config: From a0a8fc15ffe9802e608ebfc25b3eece88625e392 Mon Sep 17 00:00:00 2001 From: Paul Hooijenga Date: Fri, 27 Oct 2017 13:30:36 +0200 Subject: [PATCH 4/6] Make Not and NotIn namedtuples --- pre_commit/clientlib.py | 2 +- pre_commit/schema.py | 11 ++++------- tests/schema_test.py | 4 ++-- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/pre_commit/clientlib.py b/pre_commit/clientlib.py index 3c086cb9..c94691a5 100644 --- a/pre_commit/clientlib.py +++ b/pre_commit/clientlib.py @@ -124,7 +124,7 @@ CONFIG_REPO_DICT = schema.Map( schema.Conditional( 'sha', schema.check_string, condition_key='repo', - condition_value=schema.NotIn((_LOCAL_SENTINEL, _META_SENTINEL)), + condition_value=schema.NotIn(_LOCAL_SENTINEL, _META_SENTINEL), ensure_absent=True, ), ) diff --git a/pre_commit/schema.py b/pre_commit/schema.py index e85c2303..89e1bcfc 100644 --- a/pre_commit/schema.py +++ b/pre_commit/schema.py @@ -201,17 +201,14 @@ class Array(collections.namedtuple('Array', ('of',))): return [remove_defaults(val, self.of) for val in v] -class Not(object): - def __init__(self, val): - self.val = val - +class Not(collections.namedtuple('Not', ('val',))): def __eq__(self, other): return other is not MISSING and other != self.val -class NotIn(object): - def __init__(self, values): - self.values = values +class NotIn(collections.namedtuple('NotIn', ('values',))): + def __new__(cls, *values): + return super(NotIn, cls).__new__(cls, values=values) def __eq__(self, other): return other is not MISSING and other not in self.values diff --git a/tests/schema_test.py b/tests/schema_test.py index 06f28e76..565f7e17 100644 --- a/tests/schema_test.py +++ b/tests/schema_test.py @@ -113,7 +113,7 @@ def test_not(val, expected): (('bar', True), ('foo', False), (MISSING, False)), ) def test_not_in(values, expected): - compared = NotIn(('baz', 'foo')) + compared = NotIn('baz', 'foo') assert (values == compared) is expected assert (compared == values) is expected @@ -211,7 +211,7 @@ map_conditional_absent_not_in = Map( 'foo', 'key', Conditional( 'key2', check_bool, - condition_key='key', condition_value=NotIn((1, 2)), ensure_absent=True, + condition_key='key', condition_value=NotIn(1, 2), ensure_absent=True, ), ) From 9db827ef9d9e1dfcea5ecabfeb74b9f34fac9926 Mon Sep 17 00:00:00 2001 From: Paul Hooijenga Date: Sat, 28 Oct 2017 13:59:11 +0200 Subject: [PATCH 5/6] Refactor meta hooks --- .../meta_hooks/check_files_matches_any.py | 54 ++++---- .../meta_hooks/check_useless_excludes.py | 30 +++- pre_commit/repository.py | 72 +++++----- tests/commands/run_test.py | 98 ------------- tests/meta_hooks/__init__.py | 0 tests/meta_hooks/hook_matches_any_test.py | 130 ++++++++++++++++++ tests/meta_hooks/useless_excludes_test.py | 107 ++++++++++++++ tests/repository_test.py | 2 +- 8 files changed, 329 insertions(+), 164 deletions(-) create mode 100644 tests/meta_hooks/__init__.py create mode 100644 tests/meta_hooks/hook_matches_any_test.py create mode 100644 tests/meta_hooks/useless_excludes_test.py diff --git a/pre_commit/meta_hooks/check_files_matches_any.py b/pre_commit/meta_hooks/check_files_matches_any.py index 88b4806f..d253939e 100644 --- a/pre_commit/meta_hooks/check_files_matches_any.py +++ b/pre_commit/meta_hooks/check_files_matches_any.py @@ -1,36 +1,40 @@ -import re -import sys +import argparse import pre_commit.constants as C -from pre_commit.clientlib import load_config +from pre_commit.commands.run import _filter_by_include_exclude +from pre_commit.commands.run import _filter_by_types from pre_commit.git import get_all_files +from pre_commit.runner import Runner -def files_matches_any(filenames, include): - include_re = re.compile(include) - for filename in filenames: - if include_re.search(filename): - return True - return False - - -def check_files_matches_any(config_file=None): - config = load_config(config_file or C.CONFIG_FILE) +def check_all_hooks_match_files(config_file): + runner = Runner.create(config_file) files = get_all_files() - files_not_matched = False + files_matched = True - for repo in config['repos']: - for hook in repo['hooks']: - include = hook.get('files', '') - if include and not files_matches_any(files, include): - print( - 'The files pattern for {} does not match any files' - .format(hook['id']) - ) - files_not_matched = True + for repo in runner.repositories: + for hook_id, hook in repo.hooks: + include, exclude = hook['files'], hook['exclude'] + filtered = _filter_by_include_exclude(files, include, exclude) + types, exclude_types = hook['types'], hook['exclude_types'] + filtered = _filter_by_types(filtered, types, exclude_types) + if not filtered: + print('{} does not apply to this repository'.format(hook_id)) + files_matched = False - return files_not_matched + return files_matched + + +def main(argv=None): + parser = argparse.ArgumentParser() + parser.add_argument('filenames', nargs='*', default=[C.CONFIG_FILE]) + args = parser.parse_args(argv) + + retv = 0 + for filename in args.filenames: + retv |= not check_all_hooks_match_files(filename) + return retv if __name__ == '__main__': - sys.exit(check_files_matches_any()) + exit(main()) diff --git a/pre_commit/meta_hooks/check_useless_excludes.py b/pre_commit/meta_hooks/check_useless_excludes.py index 8e891bc1..89448d78 100644 --- a/pre_commit/meta_hooks/check_useless_excludes.py +++ b/pre_commit/meta_hooks/check_useless_excludes.py @@ -1,5 +1,7 @@ +from __future__ import print_function + +import argparse import re -import sys import pre_commit.constants as C from pre_commit.clientlib import load_config @@ -14,14 +16,17 @@ def exclude_matches_any(filenames, include, exclude): return False -def check_useless_excludes(config_file=None): - config = load_config(config_file or C.CONFIG_FILE) +def check_useless_excludes(config_file): + config = load_config(config_file) files = get_all_files() useless_excludes = False exclude = config.get('exclude') if exclude != '^$' and not exclude_matches_any(files, '', exclude): - print('The global exclude pattern does not match any files') + print( + 'The global exclude pattern {!r} does not match any files' + .format(exclude), + ) useless_excludes = True for repo in config['repos']: @@ -29,13 +34,24 @@ def check_useless_excludes(config_file=None): include, exclude = hook.get('files', ''), hook.get('exclude') if exclude and not exclude_matches_any(files, include, exclude): print( - 'The exclude pattern for {} does not match any files' - .format(hook['id']) + 'The exclude pattern {!r} for {} does not match any files' + .format(exclude, hook['id']), ) useless_excludes = True return useless_excludes +def main(argv=None): + parser = argparse.ArgumentParser() + parser.add_argument('filenames', nargs='*', default=[C.CONFIG_FILE]) + args = parser.parse_args(argv) + + retv = 0 + for filename in args.filenames: + retv |= check_useless_excludes(filename) + return retv + + if __name__ == '__main__': - sys.exit(check_useless_excludes()) + exit(main()) diff --git a/pre_commit/repository.py b/pre_commit/repository.py index 0afb5004..8adcbec0 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -252,50 +252,56 @@ class LocalRepository(Repository): class MetaRepository(LocalRepository): - # Note: the hook `entry` is passed through `shlex.split()` by the command - # runner, so to prevent issues with spaces and backslashes (on Windows) it - # must be quoted here. - meta_hooks = { - 'check-useless-excludes': { - 'name': 'Check for useless excludes', - 'files': '.pre-commit-config.yaml', - 'language': 'system', - 'entry': pipes.quote(sys.executable), - 'args': ['-m', 'pre_commit.meta_hooks.check_useless_excludes'], - }, - 'check-files-matches-any': { - 'name': 'Check hooks match any files', - 'files': '.pre-commit-config.yaml', - 'language': 'system', - 'entry': pipes.quote(sys.executable), - 'args': ['-m', 'pre_commit.meta_hooks.check_files_matches_any'], - }, - } + @cached_property + def manifest_hooks(self): + # The hooks are imported here to prevent circular imports. + from pre_commit.meta_hooks import check_files_matches_any + from pre_commit.meta_hooks import check_useless_excludes + + # Note: the hook `entry` is passed through `shlex.split()` by the + # command runner, so to prevent issues with spaces and backslashes + # (on Windows) it must be quoted here. + meta_hooks = [ + { + 'id': 'check-useless-excludes', + 'name': 'Check for useless excludes', + 'files': '.pre-commit-config.yaml', + 'language': 'system', + 'entry': pipes.quote(sys.executable), + 'args': ['-m', check_useless_excludes.__name__], + }, + { + 'id': 'check-files-matches-any', + 'name': 'Check hooks match any files', + 'files': '.pre-commit-config.yaml', + 'language': 'system', + 'entry': pipes.quote(sys.executable), + 'args': ['-m', check_files_matches_any.__name__], + }, + ] + + return { + hook['id']: apply_defaults( + validate(hook, MANIFEST_HOOK_DICT), + MANIFEST_HOOK_DICT, + ) + for hook in meta_hooks + } @cached_property def hooks(self): for hook in self.repo_config['hooks']: - if hook['id'] not in self.meta_hooks: + if hook['id'] not in self.manifest_hooks: logger.error( '`{}` is not a valid meta hook. ' 'Typo? Perhaps it is introduced in a newer version? ' - 'Often `pre-commit autoupdate` fixes this.'.format( - hook['id'], - ), + 'Often `pip install --upgrade pre-commit` fixes this.' + .format(hook['id']), ) exit(1) return tuple( - ( - hook['id'], - apply_defaults( - validate( - dict(self.meta_hooks[hook['id']], **hook), - MANIFEST_HOOK_DICT, - ), - MANIFEST_HOOK_DICT, - ), - ) + (hook['id'], _hook(self.manifest_hooks[hook['id']], hook)) for hook in self.repo_config['hooks'] ) diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index 24771d22..336222d6 100644 --- a/tests/commands/run_test.py +++ b/tests/commands/run_test.py @@ -670,104 +670,6 @@ def test_meta_hook_passes( ) -def test_useless_exclude_global( - cap_out, repo_with_passing_hook, mock_out_store_directory, -): - config = OrderedDict(( - ('exclude', 'foo'), - ( - 'repos', [ - OrderedDict(( - ('repo', 'meta'), - ( - 'hooks', ( - OrderedDict(( - ('id', 'check-useless-excludes'), - )), - ), - ), - )), - ], - ), - )) - add_config_to_repo(repo_with_passing_hook, config) - - _test_run( - cap_out, - repo_with_passing_hook, - opts={'all_files': True}, - expected_outputs=[ - b'Check for useless excludes', - b'The global exclude pattern does not match any files', - ], - expected_ret=1, - stage=False, - ) - - -def test_useless_exclude_for_hook( - cap_out, repo_with_passing_hook, mock_out_store_directory, -): - config = OrderedDict(( - ('repo', 'meta'), - ( - 'hooks', ( - OrderedDict(( - ('id', 'check-useless-excludes'), - ('exclude', 'foo'), - )), - ), - ), - )) - add_config_to_repo(repo_with_passing_hook, config) - - _test_run( - cap_out, - repo_with_passing_hook, - opts={'all_files': True}, - expected_outputs=[ - b'Check for useless excludes', - b'The exclude pattern for check-useless-excludes ' - b'does not match any files', - ], - expected_ret=1, - stage=False, - ) - - -def test_files_match_any( - cap_out, repo_with_passing_hook, mock_out_store_directory, -): - config = OrderedDict(( - ('repo', 'meta'), - ( - 'hooks', ( - OrderedDict(( - ('id', 'check-files-matches-any'), - )), - OrderedDict(( - ('id', 'check-useless-excludes'), - ('files', 'foo'), - )), - ), - ), - )) - add_config_to_repo(repo_with_passing_hook, config) - - _test_run( - cap_out, - repo_with_passing_hook, - opts={'all_files': True}, - expected_outputs=[ - b'Check hooks match any files', - b'The files pattern for check-useless-excludes ' - b'does not match any files', - ], - expected_ret=1, - stage=False, - ) - - @pytest.yield_fixture def modified_config_repo(repo_with_passing_hook): with modify_config(repo_with_passing_hook, commit=False) as config: diff --git a/tests/meta_hooks/__init__.py b/tests/meta_hooks/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/meta_hooks/hook_matches_any_test.py b/tests/meta_hooks/hook_matches_any_test.py new file mode 100644 index 00000000..92c6fc45 --- /dev/null +++ b/tests/meta_hooks/hook_matches_any_test.py @@ -0,0 +1,130 @@ +from collections import OrderedDict + +from pre_commit.meta_hooks import check_files_matches_any +from pre_commit.util import cwd +from testing.fixtures import add_config_to_repo +from testing.fixtures import git_dir + + +def test_hook_excludes_everything( + capsys, tempdir_factory, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + ('exclude', '.pre-commit-config.yaml'), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_files_matches_any.main(argv=[]) == 1 + + out, _ = capsys.readouterr() + assert 'check-useless-excludes does not apply to this repository' in out + + +def test_hook_includes_nothing( + capsys, tempdir_factory, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + ('files', 'foo'), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_files_matches_any.main(argv=[]) == 1 + + out, _ = capsys.readouterr() + assert 'check-useless-excludes does not apply to this repository' in out + + +def test_hook_types_not_matched( + capsys, tempdir_factory, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + ('types', ['python']), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_files_matches_any.main(argv=[]) == 1 + + out, _ = capsys.readouterr() + assert 'check-useless-excludes does not apply to this repository' in out + + +def test_hook_types_excludes_everything( + capsys, tempdir_factory, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + ('exclude_types', ['yaml']), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_files_matches_any.main(argv=[]) == 1 + + out, _ = capsys.readouterr() + assert 'check-useless-excludes does not apply to this repository' in out + + +def test_valid_includes( + capsys, tempdir_factory, mock_out_store_directory, +): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_files_matches_any.main(argv=[]) == 0 + + out, _ = capsys.readouterr() + assert out == '' diff --git a/tests/meta_hooks/useless_excludes_test.py b/tests/meta_hooks/useless_excludes_test.py new file mode 100644 index 00000000..8b6ba7b0 --- /dev/null +++ b/tests/meta_hooks/useless_excludes_test.py @@ -0,0 +1,107 @@ +from collections import OrderedDict + +from pre_commit.meta_hooks import check_useless_excludes +from pre_commit.util import cwd +from testing.fixtures import add_config_to_repo +from testing.fixtures import git_dir + + +def test_useless_exclude_global(capsys, tempdir_factory): + config = OrderedDict(( + ('exclude', 'foo'), + ( + 'repos', [ + OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + )), + ), + ), + )), + ], + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_useless_excludes.main(argv=[]) == 1 + + out, _ = capsys.readouterr() + assert "The global exclude pattern 'foo' does not match any files" in out + + +def test_useless_exclude_for_hook(capsys, tempdir_factory): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + ('exclude', 'foo'), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_useless_excludes.main(argv=[]) == 1 + + out, _ = capsys.readouterr() + expected = ( + "The exclude pattern 'foo' for check-useless-excludes " + "does not match any files" + ) + assert expected in out + + +def test_no_excludes(capsys, tempdir_factory): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_useless_excludes.main(argv=[]) == 0 + + out, _ = capsys.readouterr() + assert out == '' + + +def test_valid_exclude(capsys, tempdir_factory): + config = OrderedDict(( + ('repo', 'meta'), + ( + 'hooks', ( + OrderedDict(( + ('id', 'check-useless-excludes'), + ('exclude', '.pre-commit-config.yaml'), + )), + ), + ), + )) + + repo = git_dir(tempdir_factory) + add_config_to_repo(repo, config) + + with cwd(repo): + assert check_useless_excludes.main(argv=[]) == 0 + + out, _ = capsys.readouterr() + assert out == '' diff --git a/tests/repository_test.py b/tests/repository_test.py index 80489406..f7c027cd 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -729,7 +729,7 @@ def test_meta_hook_not_present(store, fake_log_handler): assert fake_log_handler.handle.call_args[0][0].msg == ( '`i-dont-exist` is not a valid meta hook. ' 'Typo? Perhaps it is introduced in a newer version? ' - 'Often `pre-commit autoupdate` fixes this.' + 'Often `pip install --upgrade pre-commit` fixes this.' ) From 5a8ca2ffbe72fc5142aa4a0a1de52f9d3f032d71 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 30 Oct 2017 09:12:48 -0700 Subject: [PATCH 6/6] Some minor fixups --- .pre-commit-config.yaml | 4 ++ pre_commit/commands/autoupdate.py | 3 +- .../meta_hooks/check_files_matches_any.py | 12 ++--- .../meta_hooks/check_useless_excludes.py | 27 +++++++---- pre_commit/repository.py | 47 +++++++++---------- tests/commands/autoupdate_test.py | 18 +++++++ tests/meta_hooks/hook_matches_any_test.py | 10 ++-- tests/meta_hooks/useless_excludes_test.py | 8 ++-- 8 files changed, 79 insertions(+), 50 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 36340642..9cd63760 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -25,3 +25,7 @@ repos: sha: v0.6.4 hooks: - id: add-trailing-comma +- repo: meta + hooks: + - id: check-useless-excludes + - id: check-files-matches-any diff --git a/pre_commit/commands/autoupdate.py b/pre_commit/commands/autoupdate.py index a80c6b40..ca0ed5e2 100644 --- a/pre_commit/commands/autoupdate.py +++ b/pre_commit/commands/autoupdate.py @@ -11,6 +11,7 @@ import pre_commit.constants as C from pre_commit import output from pre_commit.clientlib import CONFIG_SCHEMA from pre_commit.clientlib import is_local_repo +from pre_commit.clientlib import is_meta_repo from pre_commit.clientlib import load_config from pre_commit.commands.migrate_config import migrate_config from pre_commit.repository import Repository @@ -115,7 +116,7 @@ def autoupdate(runner, tags_only): input_config = load_config(runner.config_file_path) for repo_config in input_config['repos']: - if is_local_repo(repo_config): + if is_local_repo(repo_config) or is_meta_repo(repo_config): output_repos.append(repo_config) continue output.write('Updating {}...'.format(repo_config['repo'])) diff --git a/pre_commit/meta_hooks/check_files_matches_any.py b/pre_commit/meta_hooks/check_files_matches_any.py index d253939e..8c9a92d8 100644 --- a/pre_commit/meta_hooks/check_files_matches_any.py +++ b/pre_commit/meta_hooks/check_files_matches_any.py @@ -1,16 +1,16 @@ import argparse import pre_commit.constants as C +from pre_commit import git from pre_commit.commands.run import _filter_by_include_exclude from pre_commit.commands.run import _filter_by_types -from pre_commit.git import get_all_files from pre_commit.runner import Runner def check_all_hooks_match_files(config_file): runner = Runner.create(config_file) - files = get_all_files() - files_matched = True + files = git.get_all_files() + retv = 0 for repo in runner.repositories: for hook_id, hook in repo.hooks: @@ -20,9 +20,9 @@ def check_all_hooks_match_files(config_file): filtered = _filter_by_types(filtered, types, exclude_types) if not filtered: print('{} does not apply to this repository'.format(hook_id)) - files_matched = False + retv = 1 - return files_matched + return retv def main(argv=None): @@ -32,7 +32,7 @@ def main(argv=None): retv = 0 for filename in args.filenames: - retv |= not check_all_hooks_match_files(filename) + retv |= check_all_hooks_match_files(filename) return retv diff --git a/pre_commit/meta_hooks/check_useless_excludes.py b/pre_commit/meta_hooks/check_useless_excludes.py index 89448d78..189633a8 100644 --- a/pre_commit/meta_hooks/check_useless_excludes.py +++ b/pre_commit/meta_hooks/check_useless_excludes.py @@ -4,11 +4,15 @@ import argparse import re import pre_commit.constants as C +from pre_commit import git from pre_commit.clientlib import load_config -from pre_commit.git import get_all_files +from pre_commit.clientlib import MANIFEST_HOOK_DICT +from pre_commit.schema import apply_defaults def exclude_matches_any(filenames, include, exclude): + if exclude == '^$': + return True include_re, exclude_re = re.compile(include), re.compile(exclude) for filename in filenames: if include_re.search(filename) and exclude_re.search(filename): @@ -18,28 +22,31 @@ def exclude_matches_any(filenames, include, exclude): def check_useless_excludes(config_file): config = load_config(config_file) - files = get_all_files() - useless_excludes = False + files = git.get_all_files() + retv = 0 - exclude = config.get('exclude') - if exclude != '^$' and not exclude_matches_any(files, '', exclude): + exclude = config['exclude'] + if not exclude_matches_any(files, '', exclude): print( 'The global exclude pattern {!r} does not match any files' .format(exclude), ) - useless_excludes = True + retv = 1 for repo in config['repos']: for hook in repo['hooks']: - include, exclude = hook.get('files', ''), hook.get('exclude') - if exclude and not exclude_matches_any(files, include, exclude): + # Not actually a manifest dict, but this more accurately reflects + # the defaults applied during runtime + hook = apply_defaults(hook, MANIFEST_HOOK_DICT) + include, exclude = hook['files'], hook['exclude'] + if not exclude_matches_any(files, include, exclude): print( 'The exclude pattern {!r} for {} does not match any files' .format(exclude, hook['id']), ) - useless_excludes = True + retv = 1 - return useless_excludes + return retv def main(argv=None): diff --git a/pre_commit/repository.py b/pre_commit/repository.py index 8adcbec0..2eb62ecb 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -128,6 +128,12 @@ def _hook(*hook_dicts): return ret +def _hook_from_manifest_dct(dct): + dct = validate(apply_defaults(dct, MANIFEST_HOOK_DICT), MANIFEST_HOOK_DICT) + dct = _hook(dct) + return dct + + class Repository(object): def __init__(self, repo_config, store): self.repo_config = repo_config @@ -226,14 +232,8 @@ class LocalRepository(Repository): @cached_property def hooks(self): - def _from_manifest_dct(dct): - dct = validate(dct, MANIFEST_HOOK_DICT) - dct = apply_defaults(dct, MANIFEST_HOOK_DICT) - dct = _hook(dct) - return dct - return tuple( - (hook['id'], _from_manifest_dct(hook)) + (hook['id'], _hook_from_manifest_dct(hook)) for hook in self.repo_config['hooks'] ) @@ -258,33 +258,32 @@ class MetaRepository(LocalRepository): from pre_commit.meta_hooks import check_files_matches_any from pre_commit.meta_hooks import check_useless_excludes - # Note: the hook `entry` is passed through `shlex.split()` by the - # command runner, so to prevent issues with spaces and backslashes - # (on Windows) it must be quoted here. + def _make_entry(mod): + """the hook `entry` is passed through `shlex.split()` by the + command runner, so to prevent issues with spaces and backslashes + (on Windows) it must be quoted here. + """ + return '{} -m {}'.format(pipes.quote(sys.executable), mod.__name__) + meta_hooks = [ - { - 'id': 'check-useless-excludes', - 'name': 'Check for useless excludes', - 'files': '.pre-commit-config.yaml', - 'language': 'system', - 'entry': pipes.quote(sys.executable), - 'args': ['-m', check_useless_excludes.__name__], - }, { 'id': 'check-files-matches-any', 'name': 'Check hooks match any files', 'files': '.pre-commit-config.yaml', 'language': 'system', - 'entry': pipes.quote(sys.executable), - 'args': ['-m', check_files_matches_any.__name__], + 'entry': _make_entry(check_files_matches_any), + }, + { + 'id': 'check-useless-excludes', + 'name': 'Check for useless excludes', + 'files': '.pre-commit-config.yaml', + 'language': 'system', + 'entry': _make_entry(check_useless_excludes), }, ] return { - hook['id']: apply_defaults( - validate(hook, MANIFEST_HOOK_DICT), - MANIFEST_HOOK_DICT, - ) + hook['id']: _hook_from_manifest_dct(hook) for hook in meta_hooks } diff --git a/tests/commands/autoupdate_test.py b/tests/commands/autoupdate_test.py index 9ae70c64..7119c6be 100644 --- a/tests/commands/autoupdate_test.py +++ b/tests/commands/autoupdate_test.py @@ -295,6 +295,24 @@ def test_autoupdate_local_hooks_with_out_of_date_repo( assert new_config_writen['repos'][0] == local_config +def test_autoupdate_meta_hooks(tmpdir, capsys): + cfg = tmpdir.join(C.CONFIG_FILE) + cfg.write( + 'repos:\n' + '- repo: meta\n' + ' hooks:\n' + ' - id: check-useless-excludes\n', + ) + ret = autoupdate(Runner(tmpdir.strpath, C.CONFIG_FILE), tags_only=True) + assert ret == 0 + assert cfg.read() == ( + 'repos:\n' + '- repo: meta\n' + ' hooks:\n' + ' - id: check-useless-excludes\n' + ) + + def test_updates_old_format_to_new_format(tmpdir, capsys): cfg = tmpdir.join(C.CONFIG_FILE) cfg.write( diff --git a/tests/meta_hooks/hook_matches_any_test.py b/tests/meta_hooks/hook_matches_any_test.py index 92c6fc45..005cdf68 100644 --- a/tests/meta_hooks/hook_matches_any_test.py +++ b/tests/meta_hooks/hook_matches_any_test.py @@ -25,7 +25,7 @@ def test_hook_excludes_everything( add_config_to_repo(repo, config) with cwd(repo): - assert check_files_matches_any.main(argv=[]) == 1 + assert check_files_matches_any.main(()) == 1 out, _ = capsys.readouterr() assert 'check-useless-excludes does not apply to this repository' in out @@ -50,7 +50,7 @@ def test_hook_includes_nothing( add_config_to_repo(repo, config) with cwd(repo): - assert check_files_matches_any.main(argv=[]) == 1 + assert check_files_matches_any.main(()) == 1 out, _ = capsys.readouterr() assert 'check-useless-excludes does not apply to this repository' in out @@ -75,7 +75,7 @@ def test_hook_types_not_matched( add_config_to_repo(repo, config) with cwd(repo): - assert check_files_matches_any.main(argv=[]) == 1 + assert check_files_matches_any.main(()) == 1 out, _ = capsys.readouterr() assert 'check-useless-excludes does not apply to this repository' in out @@ -100,7 +100,7 @@ def test_hook_types_excludes_everything( add_config_to_repo(repo, config) with cwd(repo): - assert check_files_matches_any.main(argv=[]) == 1 + assert check_files_matches_any.main(()) == 1 out, _ = capsys.readouterr() assert 'check-useless-excludes does not apply to this repository' in out @@ -124,7 +124,7 @@ def test_valid_includes( add_config_to_repo(repo, config) with cwd(repo): - assert check_files_matches_any.main(argv=[]) == 0 + assert check_files_matches_any.main(()) == 0 out, _ = capsys.readouterr() assert out == '' diff --git a/tests/meta_hooks/useless_excludes_test.py b/tests/meta_hooks/useless_excludes_test.py index 8b6ba7b0..08b87aa8 100644 --- a/tests/meta_hooks/useless_excludes_test.py +++ b/tests/meta_hooks/useless_excludes_test.py @@ -29,7 +29,7 @@ def test_useless_exclude_global(capsys, tempdir_factory): add_config_to_repo(repo, config) with cwd(repo): - assert check_useless_excludes.main(argv=[]) == 1 + assert check_useless_excludes.main(()) == 1 out, _ = capsys.readouterr() assert "The global exclude pattern 'foo' does not match any files" in out @@ -52,7 +52,7 @@ def test_useless_exclude_for_hook(capsys, tempdir_factory): add_config_to_repo(repo, config) with cwd(repo): - assert check_useless_excludes.main(argv=[]) == 1 + assert check_useless_excludes.main(()) == 1 out, _ = capsys.readouterr() expected = ( @@ -78,7 +78,7 @@ def test_no_excludes(capsys, tempdir_factory): add_config_to_repo(repo, config) with cwd(repo): - assert check_useless_excludes.main(argv=[]) == 0 + assert check_useless_excludes.main(()) == 0 out, _ = capsys.readouterr() assert out == '' @@ -101,7 +101,7 @@ def test_valid_exclude(capsys, tempdir_factory): add_config_to_repo(repo, config) with cwd(repo): - assert check_useless_excludes.main(argv=[]) == 0 + assert check_useless_excludes.main(()) == 0 out, _ = capsys.readouterr() assert out == ''