From fc8456792346060dac053c535a9ce99aad43259e Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Fri, 4 Jan 2019 21:43:08 -0800 Subject: [PATCH] Default local / meta through cfgv --- pre_commit/clientlib.py | 83 +++++++++++++++++-- pre_commit/meta_hooks/check_hooks_apply.py | 9 -- .../meta_hooks/check_useless_excludes.py | 9 -- pre_commit/meta_hooks/helpers.py | 10 --- pre_commit/meta_hooks/identity.py | 9 -- pre_commit/repository.py | 58 ++----------- setup.py | 2 +- tests/clientlib_test.py | 17 ++++ tests/commands/autoupdate_test.py | 6 +- tests/commands/gc_test.py | 3 +- tests/repository_test.py | 15 +--- 11 files changed, 109 insertions(+), 112 deletions(-) delete mode 100644 pre_commit/meta_hooks/helpers.py diff --git a/pre_commit/clientlib.py b/pre_commit/clientlib.py index 07423c34..c5b99477 100644 --- a/pre_commit/clientlib.py +++ b/pre_commit/clientlib.py @@ -3,6 +3,8 @@ from __future__ import unicode_literals import argparse import functools +import pipes +import sys import cfgv from aspy.yaml import ordered_load @@ -88,8 +90,8 @@ def validate_manifest_main(argv=None): return ret -_LOCAL_SENTINEL = 'local' -_META_SENTINEL = 'meta' +_LOCAL = 'local' +_META = 'meta' class MigrateShaToRev(object): @@ -98,12 +100,12 @@ class MigrateShaToRev(object): return cfgv.Conditional( key, cfgv.check_string, condition_key='repo', - condition_value=cfgv.NotIn(_LOCAL_SENTINEL, _META_SENTINEL), + condition_value=cfgv.NotIn(_LOCAL, _META), ensure_absent=True, ) def check(self, dct): - if dct.get('repo') in {_LOCAL_SENTINEL, _META_SENTINEL}: + if dct.get('repo') in {_LOCAL, _META}: self._cond('rev').check(dct) self._cond('sha').check(dct) elif 'sha' in dct and 'rev' in dct: @@ -121,6 +123,61 @@ class MigrateShaToRev(object): pass +def _entry(modname): + """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 pre_commit.meta_hooks.{}'.format( + pipes.quote(sys.executable), modname, + ) + + +_meta = ( + ( + 'check-hooks-apply', ( + ('name', 'Check hooks apply to the repository'), + ('files', C.CONFIG_FILE), + ('entry', _entry('check_hooks_apply')), + ), + ), + ( + 'check-useless-excludes', ( + ('name', 'Check for useless excludes'), + ('files', C.CONFIG_FILE), + ('entry', _entry('check_useless_excludes')), + ), + ), + ( + 'identity', ( + ('name', 'identity'), + ('verbose', True), + ('entry', _entry('identity')), + ), + ), +) + +META_HOOK_DICT = cfgv.Map( + 'Hook', 'id', + *([ + cfgv.Required('id', cfgv.check_string), + cfgv.Required('id', cfgv.check_one_of(tuple(k for k, _ in _meta))), + # language must be system + cfgv.Optional('language', cfgv.check_one_of({'system'}), 'system'), + ] + [ + # default to the hook definition for the meta hooks + cfgv.ConditionalOptional(key, cfgv.check_any, value, 'id', hook_id) + for hook_id, values in _meta + for key, value in values + ] + [ + # default to the "manifest" parsing + cfgv.OptionalNoDefault(item.key, item.check_fn) + # these will always be defaulted above + if item.key in {'name', 'language', 'entry'} else + item + for item in MANIFEST_HOOK_DICT.items + ]) +) CONFIG_HOOK_DICT = cfgv.Map( 'Hook', 'id', @@ -140,7 +197,19 @@ CONFIG_REPO_DICT = cfgv.Map( 'Repository', 'repo', cfgv.Required('repo', cfgv.check_string), - cfgv.RequiredRecurse('hooks', cfgv.Array(CONFIG_HOOK_DICT)), + + cfgv.ConditionalRecurse( + 'hooks', cfgv.Array(CONFIG_HOOK_DICT), + 'repo', cfgv.NotIn(_LOCAL, _META), + ), + cfgv.ConditionalRecurse( + 'hooks', cfgv.Array(MANIFEST_HOOK_DICT), + 'repo', _LOCAL, + ), + cfgv.ConditionalRecurse( + 'hooks', cfgv.Array(META_HOOK_DICT), + 'repo', _META, + ), MigrateShaToRev(), ) @@ -154,11 +223,11 @@ CONFIG_SCHEMA = cfgv.Map( def is_local_repo(repo_entry): - return repo_entry['repo'] == _LOCAL_SENTINEL + return repo_entry['repo'] == _LOCAL def is_meta_repo(repo_entry): - return repo_entry['repo'] == _META_SENTINEL + return repo_entry['repo'] == _META class InvalidConfigError(FatalError): diff --git a/pre_commit/meta_hooks/check_hooks_apply.py b/pre_commit/meta_hooks/check_hooks_apply.py index a97830d2..b17a9d6f 100644 --- a/pre_commit/meta_hooks/check_hooks_apply.py +++ b/pre_commit/meta_hooks/check_hooks_apply.py @@ -5,18 +5,9 @@ from pre_commit import git 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.meta_hooks.helpers import make_meta_entry from pre_commit.repository import all_hooks from pre_commit.store import Store -HOOK_DICT = { - 'id': 'check-hooks-apply', - 'name': 'Check hooks apply to the repository', - 'files': C.CONFIG_FILE, - 'language': 'system', - 'entry': make_meta_entry(__name__), -} - def check_all_hooks_match_files(config_file): files = git.get_all_files() diff --git a/pre_commit/meta_hooks/check_useless_excludes.py b/pre_commit/meta_hooks/check_useless_excludes.py index 7918eb31..18b9f163 100644 --- a/pre_commit/meta_hooks/check_useless_excludes.py +++ b/pre_commit/meta_hooks/check_useless_excludes.py @@ -10,15 +10,6 @@ from pre_commit import git from pre_commit.clientlib import load_config from pre_commit.clientlib import MANIFEST_HOOK_DICT from pre_commit.commands.run import _filter_by_types -from pre_commit.meta_hooks.helpers import make_meta_entry - -HOOK_DICT = { - 'id': 'check-useless-excludes', - 'name': 'Check for useless excludes', - 'files': C.CONFIG_FILE, - 'language': 'system', - 'entry': make_meta_entry(__name__), -} def exclude_matches_any(filenames, include, exclude): diff --git a/pre_commit/meta_hooks/helpers.py b/pre_commit/meta_hooks/helpers.py deleted file mode 100644 index 7ef74861..00000000 --- a/pre_commit/meta_hooks/helpers.py +++ /dev/null @@ -1,10 +0,0 @@ -import pipes -import sys - - -def make_meta_entry(modname): - """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), modname) diff --git a/pre_commit/meta_hooks/identity.py b/pre_commit/meta_hooks/identity.py index 0cec32a0..ae7377b8 100644 --- a/pre_commit/meta_hooks/identity.py +++ b/pre_commit/meta_hooks/identity.py @@ -1,15 +1,6 @@ import sys from pre_commit import output -from pre_commit.meta_hooks.helpers import make_meta_entry - -HOOK_DICT = { - 'id': 'identity', - 'name': 'identity', - 'language': 'system', - 'verbose': True, - 'entry': make_meta_entry(__name__), -} def main(argv=None): diff --git a/pre_commit/repository.py b/pre_commit/repository.py index 7b980928..a654d082 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -6,9 +6,6 @@ import json import logging import os -from cfgv import apply_defaults -from cfgv import validate - import pre_commit.constants as C from pre_commit import five from pre_commit.clientlib import is_local_repo @@ -137,15 +134,8 @@ def _hook(*hook_dicts): return ret -def _hook_from_manifest_dct(dct): - dct = apply_defaults(dct, MANIFEST_HOOK_DICT) - dct = validate(dct, MANIFEST_HOOK_DICT) - dct = _hook(dct) - return dct - - -def _local_repository_hooks(repo_config, store): - def _local_prefix(language_name, deps): +def _non_cloned_repository_hooks(repo_config, store): + def _prefix(language_name, deps): language = languages[language_name] # pcre / pygrep / script / system / docker_image do not have # environments so they work out of the current directory @@ -154,45 +144,11 @@ def _local_repository_hooks(repo_config, store): else: return Prefix(store.make_local(deps)) - hook_dcts = [_hook_from_manifest_dct(h) for h in repo_config['hooks']] return tuple( Hook.create( repo_config['repo'], - _local_prefix(hook['language'], hook['additional_dependencies']), - hook, - ) - for hook in hook_dcts - ) - - -def _meta_repository_hooks(repo_config, store): - # imported here to prevent circular imports. - from pre_commit.meta_hooks import check_hooks_apply - from pre_commit.meta_hooks import check_useless_excludes - from pre_commit.meta_hooks import identity - - meta_hooks = [ - _hook_from_manifest_dct(mod.HOOK_DICT) - for mod in (check_hooks_apply, check_useless_excludes, identity) - ] - by_id = {hook['id']: hook for hook in meta_hooks} - - for hook in repo_config['hooks']: - if hook['id'] not in by_id: - logger.error( - '`{}` is not a valid meta hook. ' - 'Typo? Perhaps it is introduced in a newer version? ' - 'Often `pip install --upgrade pre-commit` fixes this.' - .format(hook['id']), - ) - exit(1) - - prefix = Prefix(os.getcwd()) - return tuple( - Hook.create( - repo_config['repo'], - prefix, - _hook(by_id[hook['id']], hook), + _prefix(hook['language'], hook['additional_dependencies']), + _hook(hook), ) for hook in repo_config['hooks'] ) @@ -225,10 +181,8 @@ def _cloned_repository_hooks(repo_config, store): def repository_hooks(repo_config, store): - if is_local_repo(repo_config): - return _local_repository_hooks(repo_config, store) - elif is_meta_repo(repo_config): - return _meta_repository_hooks(repo_config, store) + if is_local_repo(repo_config) or is_meta_repo(repo_config): + return _non_cloned_repository_hooks(repo_config, store) else: return _cloned_repository_hooks(repo_config, store) diff --git a/setup.py b/setup.py index f6ea719c..6cc52d10 100644 --- a/setup.py +++ b/setup.py @@ -36,7 +36,7 @@ setup( }, install_requires=[ 'aspy.yaml', - 'cfgv>=1.0.0', + 'cfgv>=1.3.0', 'identify>=1.0.0', # if this makes it into python3.8 move to extras_require 'importlib-metadata', diff --git a/tests/clientlib_test.py b/tests/clientlib_test.py index dbae4aad..1f691c2b 100644 --- a/tests/clientlib_test.py +++ b/tests/clientlib_test.py @@ -5,6 +5,7 @@ import pytest from pre_commit.clientlib import check_type_tag from pre_commit.clientlib import CONFIG_HOOK_DICT +from pre_commit.clientlib import CONFIG_REPO_DICT from pre_commit.clientlib import CONFIG_SCHEMA from pre_commit.clientlib import is_local_repo from pre_commit.clientlib import MANIFEST_SCHEMA @@ -236,3 +237,19 @@ def test_migrate_to_sha_ok(): dct = {'repo': 'a', 'rev': 'b'} MigrateShaToRev().apply_default(dct) assert dct == {'repo': 'a', 'rev': 'b'} + + +@pytest.mark.parametrize( + 'config_repo', + ( + # i-dont-exist isn't a valid hook + {'repo': 'meta', 'hooks': [{'id': 'i-dont-exist'}]}, + # invalid to set a language for a meta hook + {'repo': 'meta', 'hooks': [{'id': 'identity', 'language': 'python'}]}, + # name override must be string + {'repo': 'meta', 'hooks': [{'id': 'identity', 'name': False}]}, + ), +) +def test_meta_hook_invalid_id(config_repo): + with pytest.raises(cfgv.ValidationError): + cfgv.validate(config_repo, CONFIG_REPO_DICT) diff --git a/tests/commands/autoupdate_test.py b/tests/commands/autoupdate_test.py index 8daf986a..df7cb085 100644 --- a/tests/commands/autoupdate_test.py +++ b/tests/commands/autoupdate_test.py @@ -8,7 +8,6 @@ import pytest import pre_commit.constants as C from pre_commit import git -from pre_commit.clientlib import load_config from pre_commit.commands.autoupdate import _update_repo from pre_commit.commands.autoupdate import autoupdate from pre_commit.commands.autoupdate import RepositoryCannotBeUpdatedError @@ -17,6 +16,7 @@ from testing.auto_namedtuple import auto_namedtuple from testing.fixtures import add_config_to_repo from testing.fixtures import make_config_from_repo from testing.fixtures import make_repo +from testing.fixtures import read_config from testing.fixtures import sample_local_config from testing.fixtures import write_config from testing.util import get_resource_path @@ -319,7 +319,7 @@ def test_autoupdate_local_hooks(in_git_dir, store): config = sample_local_config() add_config_to_repo('.', config) assert autoupdate(C.CONFIG_FILE, store, tags_only=False) == 0 - new_config_writen = load_config(C.CONFIG_FILE) + new_config_writen = read_config('.') assert len(new_config_writen['repos']) == 1 assert new_config_writen['repos'][0] == config @@ -334,7 +334,7 @@ def test_autoupdate_local_hooks_with_out_of_date_repo( config = {'repos': [local_config, stale_config]} write_config('.', config) assert autoupdate(C.CONFIG_FILE, store, tags_only=False) == 0 - new_config_writen = load_config(C.CONFIG_FILE) + new_config_writen = read_config('.') assert len(new_config_writen['repos']) == 2 assert new_config_writen['repos'][0] == local_config diff --git a/tests/commands/gc_test.py b/tests/commands/gc_test.py index 2f958f67..2a018509 100644 --- a/tests/commands/gc_test.py +++ b/tests/commands/gc_test.py @@ -2,6 +2,7 @@ import os import pre_commit.constants as C from pre_commit import git +from pre_commit.clientlib import load_config from pre_commit.commands.autoupdate import autoupdate from pre_commit.commands.gc import gc from pre_commit.repository import all_hooks @@ -91,7 +92,7 @@ def test_gc_unused_local_repo_with_env(store, in_git_dir, cap_out): store.mark_config_used(C.CONFIG_FILE) # this causes the repositories to be created - all_hooks({'repos': [config]}, store) + all_hooks(load_config(C.CONFIG_FILE), store) assert _config_count(store) == 1 assert _repo_count(store) == 1 diff --git a/tests/repository_test.py b/tests/repository_test.py index eecf67b6..25fe2447 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -5,12 +5,14 @@ import os.path import re import shutil +import cfgv import mock import pytest import pre_commit.constants as C from pre_commit import five from pre_commit import parse_shebang +from pre_commit.clientlib import CONFIG_REPO_DICT from pre_commit.clientlib import load_manifest from pre_commit.languages import golang from pre_commit.languages import helpers @@ -42,6 +44,8 @@ def _norm_out(b): def _get_hook(config, store, hook_id): + config = cfgv.validate(config, CONFIG_REPO_DICT) + config = cfgv.apply_defaults(config, CONFIG_REPO_DICT) hooks = repository_hooks(config, store) install_hook_envs(hooks, store) hook, = [hook for hook in hooks if hook.id == hook_id] @@ -711,17 +715,6 @@ 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'}]} - with pytest.raises(SystemExit): - _get_hook(config, store, 'i-dont-exist') - 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 `pip install --upgrade pre-commit` 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: