From b9e5184ebd412ef2c9f1db9e09d4f2529e016eff Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sat, 21 Jan 2017 13:07:37 -0800 Subject: [PATCH] Introduce .pre-commit-hooks.yaml as a replacement for hooks.yaml --- .pre-commit-hooks.yaml | 12 ++++++++ hooks.yaml | 4 +-- pre_commit/constants.py | 4 ++- pre_commit/manifest.py | 28 +++++++++++++++---- pre_commit/repository.py | 2 +- testing/fixtures.py | 15 +++++++--- .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../resources/docker_hooks_repo/Dockerfile | 2 +- .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../hooks.yaml | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../{hooks.yaml => .pre-commit-hooks.yaml} | 0 .../.pre-commit-hooks.yaml | 5 ++++ tests/clientlib/validate_manifest_test.py | 2 +- tests/conftest.py | 6 ++++ tests/git_test.py | 8 +++--- tests/manifest_test.py | 20 ++++++++++++- tests/meta_test.py | 9 ++++++ tests/repository_test.py | 11 +++++++- 32 files changed, 107 insertions(+), 21 deletions(-) create mode 100644 .pre-commit-hooks.yaml rename testing/resources/arbitrary_bytes_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/arg_per_line_hooks_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/docker_hooks_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/failing_hook_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/{system_hook_with_spaces_repo => legacy_hooks_yaml_repo}/hooks.yaml (100%) rename testing/resources/modified_file_returns_zero_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/node_0_11_8_hooks_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/node_hooks_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/not_found_exe/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/not_installable_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/pcre_hooks_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/prints_cwd_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/python3_hooks_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/python_hooks_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/ruby_hooks_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/ruby_versioned_hooks_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/script_hooks_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) rename testing/resources/swift_hooks_repo/{hooks.yaml => .pre-commit-hooks.yaml} (100%) create mode 100644 testing/resources/system_hook_with_spaces_repo/.pre-commit-hooks.yaml create mode 100644 tests/meta_test.py diff --git a/.pre-commit-hooks.yaml b/.pre-commit-hooks.yaml new file mode 100644 index 00000000..af53043e --- /dev/null +++ b/.pre-commit-hooks.yaml @@ -0,0 +1,12 @@ +- id: validate_config + name: Validate Pre-Commit Config + description: This validator validates a pre-commit hooks config file + entry: pre-commit-validate-config + language: python + files: ^\.pre-commit-config\.yaml$ +- id: validate_manifest + name: Validate Pre-Commit Manifest + description: This validator validates a pre-commit hooks manifest file + entry: pre-commit-validate-manifest + language: python + files: ^(\.pre-commit-hooks\.yaml|hooks\.yaml)$ diff --git a/hooks.yaml b/hooks.yaml index da518028..af53043e 100644 --- a/hooks.yaml +++ b/hooks.yaml @@ -3,10 +3,10 @@ description: This validator validates a pre-commit hooks config file entry: pre-commit-validate-config language: python - files: ^\.pre-commit-config.yaml$ + files: ^\.pre-commit-config\.yaml$ - id: validate_manifest name: Validate Pre-Commit Manifest description: This validator validates a pre-commit hooks manifest file entry: pre-commit-validate-manifest language: python - files: ^hooks.yaml$ + files: ^(\.pre-commit-hooks\.yaml|hooks\.yaml)$ diff --git a/pre_commit/constants.py b/pre_commit/constants.py index b89c29f8..29ad6a03 100644 --- a/pre_commit/constants.py +++ b/pre_commit/constants.py @@ -3,7 +3,9 @@ from __future__ import unicode_literals CONFIG_FILE = '.pre-commit-config.yaml' -MANIFEST_FILE = 'hooks.yaml' +# In 0.12.0, the default file was changed to be namespaced +MANIFEST_FILE = '.pre-commit-hooks.yaml' +MANIFEST_FILE_LEGACY = 'hooks.yaml' YAML_DUMP_KWARGS = { 'default_flow_style': False, diff --git a/pre_commit/manifest.py b/pre_commit/manifest.py index 8a1c25f5..8827eccc 100644 --- a/pre_commit/manifest.py +++ b/pre_commit/manifest.py @@ -1,5 +1,6 @@ from __future__ import unicode_literals +import logging import os.path from cached_property import cached_property @@ -8,16 +9,33 @@ import pre_commit.constants as C from pre_commit.clientlib.validate_manifest import load_manifest +logger = logging.getLogger('pre_commit') + + class Manifest(object): - def __init__(self, repo_path_getter): + def __init__(self, repo_path_getter, repo_url): self.repo_path_getter = repo_path_getter + self.repo_url = repo_url @cached_property def manifest_contents(self): - manifest_path = os.path.join( - self.repo_path_getter.repo_path, C.MANIFEST_FILE, - ) - return load_manifest(manifest_path) + repo_path = self.repo_path_getter.repo_path + default_path = os.path.join(repo_path, C.MANIFEST_FILE) + legacy_path = os.path.join(repo_path, C.MANIFEST_FILE_LEGACY) + if os.path.exists(default_path): + return load_manifest(default_path) + else: + logger.warning( + '{} uses legacy {} to provide hooks.\n' + 'In newer versions, this file is called {}\n' + 'This will work in this version of pre-commit but will be ' + 'removed at a later time.\n' + 'If `pre-commit autoupdate` does not silence this warning ' + 'consider making an issue / pull request.'.format( + self.repo_url, C.MANIFEST_FILE_LEGACY, C.MANIFEST_FILE, + ) + ) + return load_manifest(legacy_path) @cached_property def hooks(self): diff --git a/pre_commit/repository.py b/pre_commit/repository.py index 129872c0..54f25e01 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -104,7 +104,7 @@ class Repository(object): @cached_property def manifest(self): - return Manifest(self.repo_path_getter) + return Manifest(self.repo_path_getter, self.repo_url) @cached_property def cmd_runner(self): diff --git a/testing/fixtures.py b/testing/fixtures.py index fdf651e8..4da32eb0 100644 --- a/testing/fixtures.py +++ b/testing/fixtures.py @@ -39,13 +39,17 @@ def make_repo(tempdir_factory, repo_source): @contextlib.contextmanager def modify_manifest(path): - """Modify the manifest yielded by this context to write to hooks.yaml.""" + """Modify the manifest yielded by this context to write to + .pre-commit-hooks.yaml. + """ manifest_path = os.path.join(path, C.MANIFEST_FILE) manifest = ordered_load(io.open(manifest_path).read()) yield manifest with io.open(manifest_path, 'w') as manifest_file: manifest_file.write(ordered_dump(manifest, **C.YAML_DUMP_KWARGS)) - cmd_output('git', 'commit', '-am', 'update hooks.yaml', cwd=path) + cmd_output( + 'git', 'commit', '-am', 'update .pre-commit-hooks.yaml', cwd=path, + ) @contextlib.contextmanager @@ -75,8 +79,11 @@ def config_with_local_hooks(): )) -def make_config_from_repo(repo_path, sha=None, hooks=None, check=True): - manifest = load_manifest(os.path.join(repo_path, C.MANIFEST_FILE)) +def make_config_from_repo( + repo_path, sha=None, hooks=None, check=True, legacy=False, +): + filename = C.MANIFEST_FILE_LEGACY if legacy else C.MANIFEST_FILE + manifest = load_manifest(os.path.join(repo_path, filename)) config = OrderedDict(( ('repo', repo_path), ('sha', sha or get_head_sha(repo_path)), diff --git a/testing/resources/arbitrary_bytes_repo/hooks.yaml b/testing/resources/arbitrary_bytes_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/arbitrary_bytes_repo/hooks.yaml rename to testing/resources/arbitrary_bytes_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/arg_per_line_hooks_repo/hooks.yaml b/testing/resources/arg_per_line_hooks_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/arg_per_line_hooks_repo/hooks.yaml rename to testing/resources/arg_per_line_hooks_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/docker_hooks_repo/hooks.yaml b/testing/resources/docker_hooks_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/docker_hooks_repo/hooks.yaml rename to testing/resources/docker_hooks_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/docker_hooks_repo/Dockerfile b/testing/resources/docker_hooks_repo/Dockerfile index acb5f54e..841b151b 100644 --- a/testing/resources/docker_hooks_repo/Dockerfile +++ b/testing/resources/docker_hooks_repo/Dockerfile @@ -1,3 +1,3 @@ FROM cogniteev/echo -CMD ["echo", "This is overwritten by the hooks.yaml 'entry'"] +CMD ["echo", "This is overwritten by the .pre-commit-hooks.yaml 'entry'"] diff --git a/testing/resources/failing_hook_repo/hooks.yaml b/testing/resources/failing_hook_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/failing_hook_repo/hooks.yaml rename to testing/resources/failing_hook_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/system_hook_with_spaces_repo/hooks.yaml b/testing/resources/legacy_hooks_yaml_repo/hooks.yaml similarity index 100% rename from testing/resources/system_hook_with_spaces_repo/hooks.yaml rename to testing/resources/legacy_hooks_yaml_repo/hooks.yaml diff --git a/testing/resources/modified_file_returns_zero_repo/hooks.yaml b/testing/resources/modified_file_returns_zero_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/modified_file_returns_zero_repo/hooks.yaml rename to testing/resources/modified_file_returns_zero_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/node_0_11_8_hooks_repo/hooks.yaml b/testing/resources/node_0_11_8_hooks_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/node_0_11_8_hooks_repo/hooks.yaml rename to testing/resources/node_0_11_8_hooks_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/node_hooks_repo/hooks.yaml b/testing/resources/node_hooks_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/node_hooks_repo/hooks.yaml rename to testing/resources/node_hooks_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/not_found_exe/hooks.yaml b/testing/resources/not_found_exe/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/not_found_exe/hooks.yaml rename to testing/resources/not_found_exe/.pre-commit-hooks.yaml diff --git a/testing/resources/not_installable_repo/hooks.yaml b/testing/resources/not_installable_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/not_installable_repo/hooks.yaml rename to testing/resources/not_installable_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/pcre_hooks_repo/hooks.yaml b/testing/resources/pcre_hooks_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/pcre_hooks_repo/hooks.yaml rename to testing/resources/pcre_hooks_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/prints_cwd_repo/hooks.yaml b/testing/resources/prints_cwd_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/prints_cwd_repo/hooks.yaml rename to testing/resources/prints_cwd_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/python3_hooks_repo/hooks.yaml b/testing/resources/python3_hooks_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/python3_hooks_repo/hooks.yaml rename to testing/resources/python3_hooks_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/python_hooks_repo/hooks.yaml b/testing/resources/python_hooks_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/python_hooks_repo/hooks.yaml rename to testing/resources/python_hooks_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/ruby_hooks_repo/hooks.yaml b/testing/resources/ruby_hooks_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/ruby_hooks_repo/hooks.yaml rename to testing/resources/ruby_hooks_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/ruby_versioned_hooks_repo/hooks.yaml b/testing/resources/ruby_versioned_hooks_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/ruby_versioned_hooks_repo/hooks.yaml rename to testing/resources/ruby_versioned_hooks_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/script_hooks_repo/hooks.yaml b/testing/resources/script_hooks_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/script_hooks_repo/hooks.yaml rename to testing/resources/script_hooks_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/swift_hooks_repo/hooks.yaml b/testing/resources/swift_hooks_repo/.pre-commit-hooks.yaml similarity index 100% rename from testing/resources/swift_hooks_repo/hooks.yaml rename to testing/resources/swift_hooks_repo/.pre-commit-hooks.yaml diff --git a/testing/resources/system_hook_with_spaces_repo/.pre-commit-hooks.yaml b/testing/resources/system_hook_with_spaces_repo/.pre-commit-hooks.yaml new file mode 100644 index 00000000..b2c347c1 --- /dev/null +++ b/testing/resources/system_hook_with_spaces_repo/.pre-commit-hooks.yaml @@ -0,0 +1,5 @@ +- id: system-hook-with-spaces + name: System hook with spaces + entry: bash -c 'echo "Hello World"' + language: system + files: \.sh$ diff --git a/tests/clientlib/validate_manifest_test.py b/tests/clientlib/validate_manifest_test.py index 63ca504c..97cfd6b0 100644 --- a/tests/clientlib/validate_manifest_test.py +++ b/tests/clientlib/validate_manifest_test.py @@ -13,7 +13,7 @@ from testing.util import is_valid_according_to_schema @pytest.mark.parametrize( ('input', 'expected_output'), ( - (['hooks.yaml'], 0), + (['.pre-commit-hooks.yaml'], 0), (['non_existent_file.yaml'], 1), ([get_resource_path('valid_yaml_but_invalid_manifest.yaml')], 1), ([get_resource_path('non_parseable_yaml_file.notyaml')], 1), diff --git a/tests/conftest.py b/tests/conftest.py index 058780bb..34f194b0 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -146,6 +146,12 @@ def log_info_mock(): yield mck +@pytest.yield_fixture +def log_warning_mock(): + with mock.patch.object(logging.getLogger('pre_commit'), 'warning') as mck: + yield mck + + class FakeStream(object): def __init__(self): self.data = io.BytesIO() diff --git a/tests/git_test.py b/tests/git_test.py index 95b9df39..c18dcd83 100644 --- a/tests/git_test.py +++ b/tests/git_test.py @@ -68,11 +68,11 @@ def test_cherry_pick_conflict(in_merge_conflict): def get_files_matching_func(): def get_filenames(): return ( + '.pre-commit-hooks.yaml', 'pre_commit/main.py', 'pre_commit/git.py', 'im_a_file_that_doesnt_exist.py', 'testing/test_symlink', - 'hooks.yaml', ) return git.get_files_matching(get_filenames) @@ -81,9 +81,9 @@ def get_files_matching_func(): def test_get_files_matching_base(get_files_matching_func): ret = get_files_matching_func('', '^$') assert ret == { + '.pre-commit-hooks.yaml', 'pre_commit/main.py', 'pre_commit/git.py', - 'hooks.yaml', 'testing/test_symlink' } @@ -95,7 +95,7 @@ def test_get_files_matching_total_match(get_files_matching_func): def test_does_search_instead_of_match(get_files_matching_func): ret = get_files_matching_func('\\.yaml$', '^$') - assert ret == {'hooks.yaml'} + assert ret == {'.pre-commit-hooks.yaml'} def test_does_not_include_deleted_fileS(get_files_matching_func): @@ -105,7 +105,7 @@ def test_does_not_include_deleted_fileS(get_files_matching_func): def test_exclude_removes_files(get_files_matching_func): ret = get_files_matching_func('', '\\.py$') - assert ret == {'hooks.yaml', 'testing/test_symlink'} + assert ret == {'.pre-commit-hooks.yaml', 'testing/test_symlink'} def resolve_conflict(): diff --git a/tests/manifest_test.py b/tests/manifest_test.py index 174f201f..e9e39dd4 100644 --- a/tests/manifest_test.py +++ b/tests/manifest_test.py @@ -13,7 +13,7 @@ def manifest(store, tempdir_factory): path = make_repo(tempdir_factory, 'script_hooks_repo') head_sha = get_head_sha(path) repo_path_getter = store.get_repo_path_getter(path, head_sha) - yield Manifest(repo_path_getter) + yield Manifest(repo_path_getter, path) def test_manifest_contents(manifest): @@ -49,3 +49,21 @@ def test_hooks(manifest): 'name': 'Bash hook', 'stages': [], } + + +def test_legacy_manifest_warn(store, tempdir_factory, log_warning_mock): + path = make_repo(tempdir_factory, 'legacy_hooks_yaml_repo') + head_sha = get_head_sha(path) + repo_path_getter = store.get_repo_path_getter(path, head_sha) + + Manifest(repo_path_getter, path).manifest_contents + + # Should have printed a warning + assert log_warning_mock.call_args_list[0][0][0] == ( + '{} uses legacy hooks.yaml to provide hooks.\n' + 'In newer versions, this file is called .pre-commit-hooks.yaml\n' + 'This will work in this version of pre-commit but will be removed at ' + 'a later time.\n' + 'If `pre-commit autoupdate` does not silence this warning consider ' + 'making an issue / pull request.'.format(path) + ) diff --git a/tests/meta_test.py b/tests/meta_test.py new file mode 100644 index 00000000..64cea262 --- /dev/null +++ b/tests/meta_test.py @@ -0,0 +1,9 @@ +import io + +import pre_commit.constants as C + + +def test_hooks_yaml_same_contents(): + legacy_contents = io.open(C.MANIFEST_FILE_LEGACY).read() + contents = io.open(C.MANIFEST_FILE).read() + assert legacy_contents == contents diff --git a/tests/repository_test.py b/tests/repository_test.py index a725934f..b7ce8dd4 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -45,7 +45,7 @@ def _test_hook_repo( args, expected, expected_return_code=0, - config_kwargs=None + config_kwargs=None, ): path = make_repo(tempdir_factory, repo_path) config = make_config_from_repo(path, **(config_kwargs or {})) @@ -215,6 +215,15 @@ def test_system_hook_with_spaces(tempdir_factory, store): ) +@pytest.mark.integration +def test_repo_with_legacy_hooks_yaml(tempdir_factory, store): + _test_hook_repo( + tempdir_factory, store, 'legacy_hooks_yaml_repo', + 'system-hook-with-spaces', ['/dev/null'], b'Hello World\n', + config_kwargs={'legacy': True}, + ) + + @skipif_cant_run_swift @pytest.mark.integration def test_swift_hook(tempdir_factory, store):