Merge pull request #907 from asottile/schema_default

Default local / meta through cfgv

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'
+_LOCAL = 'local'
-_META_SENTINEL = 'meta'
+_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):

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()

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):

pre_commit/meta_hooks/helpers.py

@@ -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)

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):

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):
+def _non_cloned_repository_hooks(repo_config, store):
-    dct = apply_defaults(dct, MANIFEST_HOOK_DICT)
+    def _prefix(language_name, deps):
-    dct = validate(dct, MANIFEST_HOOK_DICT)
-    dct = _hook(dct)
-    return dct
-
-
-def _local_repository_hooks(repo_config, store):
-    def _local_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']),
+            _prefix(hook['language'], hook['additional_dependencies']),
-            hook,
+            _hook(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),
         )
         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):
+    if is_local_repo(repo_config) or is_meta_repo(repo_config):
-        return _local_repository_hooks(repo_config, store)
+        return _non_cloned_repository_hooks(repo_config, store)
-    elif is_meta_repo(repo_config):
-        return _meta_repository_hooks(repo_config, store)
     else:
         return _cloned_repository_hooks(repo_config, store)
 

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',

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)

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
 

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

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: