From 6a0fe9889b44b863ce9c0ded88251980cfe9e6fc Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Thu, 26 Oct 2017 14:26:33 -0700 Subject: [PATCH 1/2] Apply interpreter version defaulting to local hooks too --- pre_commit/manifest.py | 9 +------- pre_commit/repository.py | 45 +++++++++++++++++++++------------------- tests/manifest_test.py | 10 --------- tests/repository_test.py | 12 +++++++++++ 4 files changed, 37 insertions(+), 39 deletions(-) diff --git a/pre_commit/manifest.py b/pre_commit/manifest.py index 10e312fb..c9caa43b 100644 --- a/pre_commit/manifest.py +++ b/pre_commit/manifest.py @@ -6,7 +6,6 @@ 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): @@ -19,10 +18,4 @@ class Manifest(object): @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 + return {hook['id']: hook for hook in self.manifest_contents} diff --git a/pre_commit/repository.py b/pre_commit/repository.py index 4d7f0a5a..b8aa1ff0 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -102,20 +102,27 @@ def _install_all(venvs, repo_url, store): _write_state(cmd_runner, venv, state) -def _validate_minimum_version(hook): - hook_version = pkg_resources.parse_version( - hook['minimum_pre_commit_version'], - ) - if hook_version > C.VERSION_PARSED: +def _hook(*hook_dicts): + ret, rest = dict(hook_dicts[0]), hook_dicts[1:] + for dct in rest: + ret.update(dct) + + version = pkg_resources.parse_version(ret['minimum_pre_commit_version']) + if version > C.VERSION_PARSED: logger.error( - 'The hook `{}` requires pre-commit version {} but ' - 'version {} is installed. ' + 'The hook `{}` requires pre-commit version {} but version {} ' + '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): @@ -161,10 +168,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'] ) @@ -215,16 +220,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'], - _validate_minimum_version( - apply_defaults( - validate(hook, MANIFEST_HOOK_DICT), - MANIFEST_HOOK_DICT, - ), - ), - ) + (hook['id'], _from_manifest_dct(hook)) for hook in self.repo_config['hooks'] ) diff --git a/tests/manifest_test.py b/tests/manifest_test.py index b7603a0d..85a3e3c6 100644 --- a/tests/manifest_test.py +++ b/tests/manifest_test.py @@ -58,13 +58,3 @@ def test_hooks(manifest): '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' diff --git a/tests/repository_test.py b/tests/repository_test.py index 37a609ba..62a3af8b 100644 --- a/tests/repository_test.py +++ b/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" From 84b1ba520d42b28c6c5f24a6fe29f27baaa33576 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Thu, 26 Oct 2017 16:08:40 -0700 Subject: [PATCH 2/2] Remove Manifest, no longer a useful abstraction --- pre_commit/commands/autoupdate.py | 2 +- pre_commit/commands/try_repo.py | 8 +++-- pre_commit/manifest.py | 21 ----------- pre_commit/repository.py | 11 +++--- tests/manifest_test.py | 60 ------------------------------- tests/repository_test.py | 26 ++++++++++++++ 6 files changed, 38 insertions(+), 90 deletions(-) delete mode 100644 pre_commit/manifest.py delete mode 100644 tests/manifest_test.py diff --git a/pre_commit/commands/autoupdate.py b/pre_commit/commands/autoupdate.py index 4dce674f..a80c6b40 100644 --- a/pre_commit/commands/autoupdate.py +++ b/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' diff --git a/pre_commit/commands/try_repo.py b/pre_commit/commands/try_repo.py index 2e1933d8..4c825823 100644 --- a/pre_commit/commands/try_repo.py +++ b/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)) - hooks = [{'id': hook_id} for hook_id in sorted(manifest.hooks)] + repo_path = Store(tempdir).clone(args.repo, ref) + 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)]} diff --git a/pre_commit/manifest.py b/pre_commit/manifest.py deleted file mode 100644 index c9caa43b..00000000 --- a/pre_commit/manifest.py +++ /dev/null @@ -1,21 +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 - - -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): - return {hook['id']: hook for hook in self.manifest_contents} diff --git a/pre_commit/repository.py b/pre_commit/repository.py index b8aa1ff0..d7af2e21 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -14,10 +14,10 @@ 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 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 @@ -152,13 +152,14 @@ class Repository(object): return self._cmd_runner @cached_property - def manifest(self): - return Manifest(self._repo_path) + def manifest_hooks(self): + 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? ' @@ -169,7 +170,7 @@ class Repository(object): exit(1) return tuple( - (hook['id'], _hook(self.manifest.hooks[hook['id']], hook)) + (hook['id'], _hook(self.manifest_hooks[hook['id']], hook)) for hook in self.repo_config['hooks'] ) diff --git a/tests/manifest_test.py b/tests/manifest_test.py deleted file mode 100644 index 85a3e3c6..00000000 --- a/tests/manifest_test.py +++ /dev/null @@ -1,60 +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': [], - } diff --git a/tests/repository_test.py b/tests/repository_test.py index 62a3af8b..fee76d87 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -746,3 +746,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': [], + }