Merge pull request #470 from pre-commit/pre_commit_hooks_yaml

Introduce .pre-commit-hooks.yaml as a replacement for hooks.yaml

.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)$

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

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,

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

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

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

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'"]

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$

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

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

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

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

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

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