diff --git a/pre_commit/clientlib.py b/pre_commit/clientlib.py index 51f14d26e..f25820200 100644 --- a/pre_commit/clientlib.py +++ b/pre_commit/clientlib.py @@ -72,11 +72,20 @@ def transform_stage(stage: str) -> str: return _STAGES.get(stage, stage) -MINIMAL_MANIFEST_SCHEMA = cfgv.Array( - cfgv.Map( - 'Hook', 'id', - cfgv.Required('id', cfgv.check_string), - cfgv.Optional('stages', cfgv.check_array(cfgv.check_string), []), +_MINIMAL_MANIFEST_SCHEMA = cfgv.Map( + 'Manifest', None, + cfgv.RequiredRecurse( + 'hooks', + 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, - load_strategy=yaml_load, + schema=_MINIMAL_MANIFEST_SCHEMA, + 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 stage in hook.get('stages', ()): + for hook in manifest['hooks'].values(): + for stage in hook['stages']: if stage in _STAGES: legacy_stages[stage] = True @@ -228,7 +237,7 @@ def check(self, dct: dict[str, Any]) -> None: 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 @@ def check(self, dct: dict[str, Any]) -> None: '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 @@ def check(self, dct: dict[str, Any]) -> None: 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): - check_min_version('5') - raise AssertionError('unreachable') + if isinstance(obj, list): + cfgv.validate(obj, _MINIMAL_LEGACY_MANIFEST_SCHEMA) + 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 @@ def check(self, dct: dict[str, Any]) -> None: *( 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 @@ def check(self, dct: dict[str, Any]) -> None: LOCAL_HOOK_DICT = cfgv.Map( 'Hook', 'id', + cfgv.Required('id', cfgv.check_string), *MANIFEST_HOOK_DICT.items, *_COMMON_HOOK_WARNINGS, ) diff --git a/pre_commit/commands/autoupdate.py b/pre_commit/commands/autoupdate.py index aa0c5e25e..838d3642e 100644 --- a/pre_commit/commands/autoupdate.py +++ b/pre_commit/commands/autoupdate.py @@ -77,7 +77,7 @@ def update(self, tags_only: bool, freeze: bool) -> RevInfo: 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) diff --git a/pre_commit/commands/gc.py b/pre_commit/commands/gc.py index 975d5e4c1..7ebf4a1ef 100644 --- a/pre_commit/commands/gc.py +++ b/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: diff --git a/pre_commit/commands/try_repo.py b/pre_commit/commands/try_repo.py index 539ed3c2b..8cf93907f 100644 --- a/pre_commit/commands/try_repo.py +++ b/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']) - hooks = [{'id': hook['id']} for hook in manifest] + hook_ids = sorted(hook['id'] for hook in manifest['hooks']) + hooks = [{'id': hook_id} for hook_id in hook_ids] config = {'repos': [{'repo': repo, 'rev': ref, 'hooks': hooks}]} config_s = yaml_dump(config) diff --git a/pre_commit/repository.py b/pre_commit/repository.py index a9461ab63..fd58b05e4 100644 --- a/pre_commit/repository.py +++ b/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: diff --git a/setup.cfg b/setup.cfg index be031c3ef..e8055fd30 100644 --- a/setup.cfg +++ b/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 diff --git a/testing/fixtures.py b/testing/fixtures.py index 79a11605e..c492d6a4d 100644 --- a/testing/fixtures.py +++ b/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: diff --git a/tests/clientlib_test.py b/tests/clientlib_test.py index 2c42b80cf..51c21b825 100644 --- a/tests/clientlib_test.py +++ b/tests/clientlib_test.py @@ -12,7 +12,6 @@ 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', - 'name': 'b', - 'entry': 'c', - 'language': 'python', - 'files': r'\.py$', - }], - [{ - 'id': 'a', - 'name': 'b', - 'entry': 'c', - 'language': 'python', - 'language_version': 'python3.4', - 'files': r'\.py$', - }], + { + 'hooks': { + 'a': { + 'name': 'b', + 'entry': 'c', + 'language': 'python', + 'files': r'\.py$', + }, + }, + }, + { + 'hooks': { + 'a': { + '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', - 'name': 'b', - 'entry': 'c', - 'language': 'python', - 'files': '', - 'always_run': True, - }], + { + 'hooks': { + 'a': { + 'id': 'a', + 'name': 'b', + 'entry': 'c', + '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: {}') - - with pytest.raises(InvalidManifestError) as excinfo: - load_manifest(manifest) - assert str(excinfo.value) == ( - f'\n' - f'==> File {manifest}\n' - f'=====> \n' - f'=====> pre-commit version 5 is required but version {C.VERSION} ' - f'is installed. Perhaps run `pip install --upgrade pre-commit`.' - ) + manifest.write_text(src) + + ret = load_manifest(manifest) + + # just to make the assertion easier + assert 'id' not in ret['hooks']['example'] + for k in tuple(ret['hooks']['example']): + if k not in {'name', 'language', 'entry'}: + ret['hooks']['example'].pop(k) + + assert ret == { + 'minimum_pre_commit_version': '0', + 'hooks': { + 'example': { + 'name': 'example', + 'language': 'unsupported', + 'entry': 'echo', + }, + }, + } diff --git a/tests/repository_test.py b/tests/repository_test.py index b1c7a0024..be64e9ecf 100644 --- a/tests/repository_test.py +++ b/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} diff --git a/tests/store_test.py b/tests/store_test.py index 13f198ea2..5b46a9b4d 100644 --- a/tests/store_test.py +++ b/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, ):