add support for top-level map manifest

pre_commit/clientlib.py

@@ -72,11 +72,20 @@ def transform_stage(stage: str) -> str:
     return _STAGES.get(stage, stage)
 
 
-MINIMAL_MANIFEST_SCHEMA = cfgv.Array(
+_MINIMAL_MANIFEST_SCHEMA = cfgv.Map(
-    cfgv.Map(
+    'Manifest', None,
-        'Hook', 'id',
+    cfgv.RequiredRecurse(
-        cfgv.Required('id', cfgv.check_string),
+        'hooks',
-        cfgv.Optional('stages', cfgv.check_array(cfgv.check_string), []),
+        cfgv.KeyValueMap(
+            'Hooks',
+            cfgv.check_string,
+            cfgv.Map(
+                'Hook', None,
+                cfgv.Optional(
+                    'stages', cfgv.check_array(cfgv.check_string), [],
+                ),
+            ),
+        ),
     ),
 )
 
@@ -85,16 +94,16 @@ def warn_for_stages_on_repo_init(repo: str, directory: str) -> None:
     try:
         manifest = cfgv.load_from_filename(
             os.path.join(directory, C.MANIFEST_FILE),
-            schema=MINIMAL_MANIFEST_SCHEMA,
+            schema=_MINIMAL_MANIFEST_SCHEMA,
-            load_strategy=yaml_load,
+            load_strategy=_load_manifest_backward_compat,
             exc_tp=InvalidManifestError,
         )
     except InvalidManifestError:
         return  # they'll get a better error message when it actually loads!
 
     legacy_stages = {}  # sorted set
-    for hook in manifest:
+    for hook in manifest['hooks'].values():
-        for stage in hook.get('stages', ()):
+        for stage in hook['stages']:
             if stage in _STAGES:
                 legacy_stages[stage] = True
 
@@ -228,7 +237,7 @@ class LanguageMigrationRequired(LanguageMigration):  # replace with Required
 
 
 MANIFEST_HOOK_DICT = cfgv.Map(
-    'Hook', 'id',
+    'Hook', None,
 
     # check first in case it uses some newer, incompatible feature
     cfgv.Optional(
@@ -237,7 +246,6 @@ MANIFEST_HOOK_DICT = cfgv.Map(
         '0',
     ),
 
-    cfgv.Required('id', cfgv.check_string),
     cfgv.Required('name', cfgv.check_string),
     cfgv.Required('entry', cfgv.check_string),
     LanguageMigrationRequired('language', cfgv.check_one_of(language_names)),
@@ -263,18 +271,38 @@ MANIFEST_HOOK_DICT = cfgv.Map(
     StagesMigration('stages', []),
     cfgv.Optional('verbose', cfgv.check_bool, False),
 )
-MANIFEST_SCHEMA = cfgv.Array(MANIFEST_HOOK_DICT)
+
+
+MANIFEST_SCHEMA = cfgv.Map(
+    'Manifest', None,
+
+    # check first in case it uses some newer, incompatible feature
+    cfgv.Optional(
+        'minimum_pre_commit_version',
+        cfgv.check_and(cfgv.check_string, check_min_version),
+        '0',
+    ),
+    cfgv.RequiredRecurse(
+        'hooks',
+        cfgv.KeyValueMap('Hooks', cfgv.check_string, MANIFEST_HOOK_DICT),
+    ),
+)
 
 
 class InvalidManifestError(FatalError):
     pass
 
 
-def _load_manifest_forward_compat(contents: str) -> object:
+_MINIMAL_LEGACY_MANIFEST_SCHEMA = cfgv.Array(
+    cfgv.Map('Hook', 'id', cfgv.Required('id', cfgv.check_string)),
+)
+
+
+def _load_manifest_backward_compat(contents: str) -> object:
     obj = yaml_load(contents)
-    if isinstance(obj, dict):
+    if isinstance(obj, list):
-        check_min_version('5')
+        cfgv.validate(obj, _MINIMAL_LEGACY_MANIFEST_SCHEMA)
-        raise AssertionError('unreachable')
+        return {'hooks': {hook.pop('id'): hook for hook in obj}}
     else:
         return obj
 
@@ -282,7 +310,7 @@ def _load_manifest_forward_compat(contents: str) -> object:
 load_manifest = functools.partial(
     cfgv.load_from_filename,
     schema=MANIFEST_SCHEMA,
-    load_strategy=_load_manifest_forward_compat,
+    load_strategy=_load_manifest_backward_compat,
     exc_tp=InvalidManifestError,
 )
 
@@ -448,7 +476,6 @@ CONFIG_HOOK_DICT = cfgv.Map(
     *(
         cfgv.OptionalNoDefault(item.key, item.check_fn)
         for item in MANIFEST_HOOK_DICT.items
-        if item.key != 'id'
         if item.key != 'stages'
         if item.key != 'language'  # remove
     ),
@@ -459,6 +486,7 @@ CONFIG_HOOK_DICT = cfgv.Map(
 LOCAL_HOOK_DICT = cfgv.Map(
     'Hook', 'id',
 
+    cfgv.Required('id', cfgv.check_string),
     *MANIFEST_HOOK_DICT.items,
     *_COMMON_HOOK_WARNINGS,
 )

pre_commit/commands/autoupdate.py

@@ -77,7 +77,7 @@ class RevInfo(NamedTuple):
             except InvalidManifestError as e:
                 raise RepositoryCannotBeUpdatedError(f'[{self.repo}] {e}')
             else:
-                hook_ids = frozenset(hook['id'] for hook in manifest)
+                hook_ids = frozenset(hook['id'] for hook in manifest['hooks'])
 
         return self._replace(rev=rev, frozen=frozen, hook_ids=hook_ids)
 

pre_commit/commands/gc.py

@@ -43,7 +43,7 @@ def _mark_used_repos(
             return
         else:
             unused_repos.discard(key)
-            by_id = {hook['id']: hook for hook in manifest}
+            by_id = manifest['hooks']
 
         for hook in repo['hooks']:
             if hook['id'] not in by_id:

pre_commit/commands/try_repo.py

@@ -58,8 +58,8 @@ def try_repo(args: argparse.Namespace) -> int:
         else:
             repo_path = store.clone(repo, ref)
             manifest = load_manifest(os.path.join(repo_path, C.MANIFEST_FILE))
-            manifest = sorted(manifest, key=lambda hook: hook['id'])
+            hook_ids = sorted(hook['id'] for hook in manifest['hooks'])
-            hooks = [{'id': hook['id']} for hook in manifest]
+            hooks = [{'id': hook_id} for hook_id in hook_ids]
 
         config = {'repos': [{'repo': repo, 'rev': ref, 'hooks': hooks}]}
         config_s = yaml_dump(config)

pre_commit/repository.py

@@ -175,7 +175,7 @@ def _cloned_repository_hooks(
 ) -> tuple[Hook, ...]:
     repo, rev = repo_config['repo'], repo_config['rev']
     manifest_path = os.path.join(store.clone(repo, rev), C.MANIFEST_FILE)
-    by_id = {hook['id']: hook for hook in load_manifest(manifest_path)}
+    by_id = load_manifest(manifest_path)['hooks']
 
     for hook in repo_config['hooks']:
         if hook['id'] not in by_id:

setup.cfg

@@ -18,7 +18,7 @@ classifiers =
 [options]
 packages = find:
 install_requires =
-    cfgv>=2.0.0
+    cfgv>=3.5.0
     identify>=1.0.0
     nodeenv>=0.11.1
     pyyaml>=5.1

testing/fixtures.py

@@ -101,7 +101,7 @@ def make_config_from_repo(repo_path, rev=None, hooks=None, check=True):
     config = {
         'repo': f'file://{repo_path}',
         'rev': rev or git.head_rev(repo_path),
-        'hooks': hooks or [{'id': hook['id']} for hook in manifest],
+        'hooks': hooks or [{'id': hook_id} for hook_id in manifest['hooks']],
     }
 
     if check:

tests/clientlib_test.py

@@ -12,7 +12,6 @@ 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 DEFAULT_LANGUAGE_VERSION
-from pre_commit.clientlib import InvalidManifestError
 from pre_commit.clientlib import load_manifest
 from pre_commit.clientlib import MANIFEST_HOOK_DICT
 from pre_commit.clientlib import MANIFEST_SCHEMA
@@ -405,30 +404,40 @@ def test_unsupported_language_migration_language_required():
 @pytest.mark.parametrize(
     'manifest_obj',
     (
-        [{
+        {
-            'id': 'a',
+            'hooks': {
-            'name': 'b',
+                'a': {
-            'entry': 'c',
+                    'name': 'b',
-            'language': 'python',
+                    'entry': 'c',
-            'files': r'\.py$',
+                    'language': 'python',
-        }],
+                    'files': r'\.py$',
-        [{
+                },
-            'id': 'a',
+            },
-            'name': 'b',
+        },
-            'entry': 'c',
+        {
-            'language': 'python',
+            'hooks': {
-            'language_version': 'python3.4',
+                'a': {
-            'files': r'\.py$',
+                    'name': 'b',
-        }],
+                    'entry': 'c',
+                    'language': 'python',
+                    'language_version': 'python3.4',
+                    'files': r'\.py$',
+                },
+            },
+        },
         # A regression in 0.13.5: always_run and files are permissible
-        [{
+        {
-            'id': 'a',
+            'hooks': {
-            'name': 'b',
+                'a': {
-            'entry': 'c',
+                    'id': 'a',
-            'language': 'python',
+                    'name': 'b',
-            'files': '',
+                    'entry': 'c',
-            'always_run': True,
+                    'language': 'python',
-        }],
+                    'files': '',
+                    'always_run': True,
+                },
+            },
+        },
     ),
 )
 def test_valid_manifests(manifest_obj):
@@ -592,16 +601,31 @@ def test_config_hook_stages_defaulting():
     }
 
 
-def test_manifest_v5_forward_compat(tmp_path):
+def test_manifest_backward_compat(tmp_path):
+    src = '''\
+-   id: example
+    name: example
+    language: unsupported
+    entry: echo
+'''
     manifest = tmp_path.joinpath('.pre-commit-hooks.yaml')
-    manifest.write_text('hooks: {}')
+    manifest.write_text(src)
 
-    with pytest.raises(InvalidManifestError) as excinfo:
+    ret = load_manifest(manifest)
-        load_manifest(manifest)
+
-    assert str(excinfo.value) == (
+    # just to make the assertion easier
-        f'\n'
+    assert 'id' not in ret['hooks']['example']
-        f'==> File {manifest}\n'
+    for k in tuple(ret['hooks']['example']):
-        f'=====> \n'
+        if k not in {'name', 'language', 'entry'}:
-        f'=====> pre-commit version 5 is required but version {C.VERSION} '
+            ret['hooks']['example'].pop(k)
-        f'is installed.  Perhaps run `pip install --upgrade pre-commit`.'
+
-    )
+    assert ret == {
+        'minimum_pre_commit_version': '0',
+        'hooks': {
+            'example': {
+                'name': 'example',
+                'language': 'unsupported',
+                'entry': 'echo',
+            },
+        },
+    }

tests/repository_test.py

@@ -344,7 +344,8 @@ def local_python_config():
     repo_path = get_resource_path('python_hooks_repo')
     manifest = load_manifest(os.path.join(repo_path, C.MANIFEST_FILE))
     hooks = [
-        dict(hook, additional_dependencies=[repo_path]) for hook in manifest
+        {'id': hook_id, **hook, 'additional_dependencies': [repo_path]}
+        for hook_id, hook in manifest['hooks'].items()
     ]
     return {'repo': 'local', 'hooks': hooks}
 

tests/store_test.py

@@ -144,6 +144,48 @@ def test_warning_for_deprecated_stages_on_init(store, tempdir_factory, caplog):
     assert caplog.record_tuples == []
 
 
+def test_warning_for_deprecated_stages_v5_manifest(
+        store, tempdir_factory, caplog,
+):
+    manifest = '''\
+hooks:
+    hook1:
+        name: hook1
+        language: system
+        entry: echo hook1
+        stages: [commit, push]
+    hook2:
+        name: hook2
+        language: system
+        entry: echo hook2
+        stages: [push, merge-commit]
+'''
+
+    path = git_dir(tempdir_factory)
+    with open(os.path.join(path, C.MANIFEST_FILE), 'w') as f:
+        f.write(manifest)
+    cmd_output('git', 'add', '.', cwd=path)
+    git_commit(cwd=path)
+    rev = git.head_rev(path)
+
+    store.clone(path, rev)
+    assert caplog.record_tuples[1] == (
+        'pre_commit',
+        logging.WARNING,
+        f'repo `{path}` uses deprecated stage names '
+        f'(commit, push, merge-commit) which will be removed in a future '
+        f'version.  '
+        f'Hint: often `pre-commit autoupdate --repo {shlex.quote(path)}` '
+        f'will fix this.  '
+        f'if it does not -- consider reporting an issue to that repo.',
+    )
+
+    # should not re-warn
+    caplog.clear()
+    store.clone(path, rev)
+    assert caplog.record_tuples == []
+
+
 def test_no_warning_for_non_deprecated_stages_on_init(
         store, tempdir_factory, caplog,
 ):