Merge branch 'master' into meta-hooks

pre_commit/commands/autoupdate.py

@@ -57,7 +57,7 @@ def _update_repo(repo_config, runner, tags_only):
 
     # See if any of our hooks were deleted with the new commits
     hooks = {hook['id'] for hook in repo.repo_config['hooks']}
-    hooks_missing = hooks - (hooks & set(new_repo.manifest.hooks))
+    hooks_missing = hooks - (hooks & set(new_repo.manifest_hooks))
     if hooks_missing:
         raise RepositoryCannotBeUpdatedError(
             'Cannot update because the tip of master is missing these hooks:\n'

pre_commit/commands/try_repo.py

@@ -9,8 +9,8 @@ from aspy.yaml import ordered_dump
 import pre_commit.constants as C
 from pre_commit import git
 from pre_commit import output
+from pre_commit.clientlib import load_manifest
 from pre_commit.commands.run import run
-from pre_commit.manifest import Manifest
 from pre_commit.runner import Runner
 from pre_commit.store import Store
 from pre_commit.util import tmpdir
@@ -23,8 +23,10 @@ def try_repo(args):
         if args.hook:
             hooks = [{'id': args.hook}]
         else:
-            manifest = Manifest(Store(tempdir).clone(args.repo, ref))
+            repo_path = Store(tempdir).clone(args.repo, ref)
-            hooks = [{'id': hook_id} for hook_id in sorted(manifest.hooks)]
+            manifest = load_manifest(os.path.join(repo_path, C.MANIFEST_FILE))
+            manifest = sorted(manifest, key=lambda hook: hook['id'])
+            hooks = [{'id': hook['id']} for hook in manifest]
 
         items = (('repo', args.repo), ('sha', ref), ('hooks', hooks))
         config = {'repos': [collections.OrderedDict(items)]}

pre_commit/manifest.py

@@ -1,28 +0,0 @@
-from __future__ import unicode_literals
-
-import os.path
-
-from cached_property import cached_property
-
-import pre_commit.constants as C
-from pre_commit.clientlib import load_manifest
-from pre_commit.languages.all import languages
-
-
-class Manifest(object):
-    def __init__(self, repo_path):
-        self.repo_path = repo_path
-
-    @cached_property
-    def manifest_contents(self):
-        return load_manifest(os.path.join(self.repo_path, C.MANIFEST_FILE))
-
-    @cached_property
-    def hooks(self):
-        ret = {}
-        for hook in self.manifest_contents:
-            if hook['language_version'] == 'default':
-                language = languages[hook['language']]
-                hook['language_version'] = language.get_default_version()
-            ret[hook['id']] = hook
-        return ret

pre_commit/repository.py

@@ -17,10 +17,10 @@ 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 load_manifest
 from pre_commit.clientlib import MANIFEST_HOOK_DICT
 from pre_commit.languages.all import languages
 from pre_commit.languages.helpers import environment_dir
-from pre_commit.manifest import Manifest
 from pre_commit.prefixed_command_runner import PrefixedCommandRunner
 from pre_commit.schema import apply_defaults
 from pre_commit.schema import validate
@@ -105,20 +105,27 @@ def _install_all(venvs, repo_url, store):
             _write_state(cmd_runner, venv, state)
 
 
-def _validate_minimum_version(hook):
+def _hook(*hook_dicts):
-    hook_version = pkg_resources.parse_version(
+    ret, rest = dict(hook_dicts[0]), hook_dicts[1:]
-        hook['minimum_pre_commit_version'],
+    for dct in rest:
-    )
+        ret.update(dct)
-    if hook_version > C.VERSION_PARSED:
+
+    version = pkg_resources.parse_version(ret['minimum_pre_commit_version'])
+    if version > C.VERSION_PARSED:
         logger.error(
-            'The hook `{}` requires pre-commit version {} but '
+            'The hook `{}` requires pre-commit version {} but version {} '
-            'version {} is installed.  '
+            'is installed.  '
             'Perhaps run `pip install --upgrade pre-commit`.'.format(
-                hook['id'], hook_version, C.VERSION_PARSED,
+                ret['id'], version, C.VERSION_PARSED,
             ),
         )
         exit(1)
-    return hook
+
+    if ret['language_version'] == 'default':
+        language = languages[ret['language']]
+        ret['language_version'] = language.get_default_version()
+
+    return ret
 
 
 class Repository(object):
@@ -150,13 +157,14 @@ class Repository(object):
         return self._cmd_runner
 
     @cached_property
-    def manifest(self):
+    def manifest_hooks(self):
-        return Manifest(self._repo_path)
+        manifest_path = os.path.join(self._repo_path, C.MANIFEST_FILE)
+        return {hook['id']: hook for hook in load_manifest(manifest_path)}
 
     @cached_property
     def hooks(self):
         for hook in self.repo_config['hooks']:
-            if hook['id'] not in self.manifest.hooks:
+            if hook['id'] not in self.manifest_hooks:
                 logger.error(
                     '`{}` is not present in repository {}.  '
                     'Typo? Perhaps it is introduced in a newer version?  '
@@ -166,10 +174,8 @@ class Repository(object):
                 )
                 exit(1)
 
-            _validate_minimum_version(self.manifest.hooks[hook['id']])
-
         return tuple(
-            (hook['id'], dict(self.manifest.hooks[hook['id']], **hook))
+            (hook['id'], _hook(self.manifest_hooks[hook['id']], hook))
             for hook in self.repo_config['hooks']
         )
 
@@ -220,16 +226,14 @@ 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'],
-                _validate_minimum_version(
-                    apply_defaults(
-                        validate(hook, MANIFEST_HOOK_DICT),
-                        MANIFEST_HOOK_DICT,
-                    ),
-                ),
-            )
             for hook in self.repo_config['hooks']
         )
 

tests/manifest_test.py

@@ -1,70 +0,0 @@
-from __future__ import absolute_import
-from __future__ import unicode_literals
-
-import pytest
-
-from pre_commit import git
-from pre_commit.manifest import Manifest
-from testing.fixtures import make_repo
-
-
-@pytest.yield_fixture
-def manifest(store, tempdir_factory):
-    path = make_repo(tempdir_factory, 'script_hooks_repo')
-    repo_path = store.clone(path, git.head_sha(path))
-    yield Manifest(repo_path)
-
-
-def test_manifest_contents(manifest):
-    # Should just retrieve the manifest contents
-    assert manifest.manifest_contents == [{
-        'always_run': False,
-        'additional_dependencies': [],
-        'args': [],
-        'description': '',
-        'entry': 'bin/hook.sh',
-        'exclude': '^$',
-        'files': '',
-        'id': 'bash_hook',
-        'language': 'script',
-        'language_version': 'default',
-        'log_file': '',
-        'minimum_pre_commit_version': '0',
-        'name': 'Bash hook',
-        'pass_filenames': True,
-        'stages': [],
-        'types': ['file'],
-        'exclude_types': [],
-    }]
-
-
-def test_hooks(manifest):
-    assert manifest.hooks['bash_hook'] == {
-        'always_run': False,
-        'additional_dependencies': [],
-        'args': [],
-        'description': '',
-        'entry': 'bin/hook.sh',
-        'exclude': '^$',
-        'files': '',
-        'id': 'bash_hook',
-        'language': 'script',
-        'language_version': 'default',
-        'log_file': '',
-        'minimum_pre_commit_version': '0',
-        'name': 'Bash hook',
-        'pass_filenames': True,
-        'stages': [],
-        'types': ['file'],
-        'exclude_types': [],
-    }
-
-
-def test_default_python_language_version(store, tempdir_factory):
-    path = make_repo(tempdir_factory, 'python_hooks_repo')
-    repo_path = store.clone(path, git.head_sha(path))
-    manifest = Manifest(repo_path)
-
-    # This assertion is difficult as it is version dependent, just assert
-    # that it is *something*
-    assert manifest.hooks['foo']['language_version'] != 'default'

tests/repository_test.py

@@ -71,6 +71,16 @@ def test_python_hook(tempdir_factory, store):
     )
 
 
+@pytest.mark.integration
+def test_python_hook_default_version(tempdir_factory, store):
+    # make sure that this continues to work for platforms where default
+    # language detection does not work
+    with mock.patch.object(
+            python, 'get_default_version', return_value='default',
+    ):
+        test_python_hook(tempdir_factory, store)
+
+
 @pytest.mark.integration
 def test_python_hook_args_with_spaces(tempdir_factory, store):
     _test_hook_repo(
@@ -690,6 +700,8 @@ def test_local_python_repo(store):
     config = {'repo': 'local', 'hooks': hooks}
     repo = Repository.create(config, store)
     (_, hook), = repo.hooks
+    # language_version should have been adjusted to the interpreter version
+    assert hook['language_version'] != 'default'
     ret = repo.run_hook(hook, ('filename',))
     assert ret[0] == 0
     assert _norm_out(ret[1]) == b"['filename']\nHello World\n"
@@ -746,3 +758,29 @@ def test_versions_ok(tempdir_factory, store, version):
     config = make_config_from_repo(path)
     # Should succeed
     Repository.create(config, store).require_installed()
+
+
+def test_manifest_hooks(tempdir_factory, store):
+    path = make_repo(tempdir_factory, 'script_hooks_repo')
+    config = make_config_from_repo(path)
+    repo = Repository.create(config, store)
+
+    assert repo.manifest_hooks['bash_hook'] == {
+        'always_run': False,
+        'additional_dependencies': [],
+        'args': [],
+        'description': '',
+        'entry': 'bin/hook.sh',
+        'exclude': '^$',
+        'files': '',
+        'id': 'bash_hook',
+        'language': 'script',
+        'language_version': 'default',
+        'log_file': '',
+        'minimum_pre_commit_version': '0',
+        'name': 'Bash hook',
+        'pass_filenames': True,
+        'stages': [],
+        'types': ['file'],
+        'exclude_types': [],
+    }