Merge pull request #3092 from pre-commit/minimum-version-first

attempt minimum_pre_commit_version first when parsing configs

pre_commit/clientlib.py

@@ -102,6 +102,13 @@ def apply_default(self, dct: dict[str, Any]) -> None:
 MANIFEST_HOOK_DICT = cfgv.Map(
     'Hook', 'id',
 
+    # 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.Required('id', cfgv.check_string),
     cfgv.Required('name', cfgv.check_string),
     cfgv.Required('entry', cfgv.check_string),
@@ -124,7 +131,6 @@ def apply_default(self, dct: dict[str, Any]) -> None:
     cfgv.Optional('description', cfgv.check_string, ''),
     cfgv.Optional('language_version', cfgv.check_string, C.DEFAULT),
     cfgv.Optional('log_file', cfgv.check_string, ''),
-    cfgv.Optional('minimum_pre_commit_version', cfgv.check_string, '0'),
     cfgv.Optional('require_serial', cfgv.check_bool, False),
     StagesMigration('stages', []),
     cfgv.Optional('verbose', cfgv.check_bool, False),
@@ -345,6 +351,13 @@ def check(self, dct: dict[str, Any]) -> None:
 CONFIG_SCHEMA = cfgv.Map(
     'Config', 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('repos', cfgv.Array(CONFIG_REPO_DICT)),
     cfgv.Optional(
         'default_install_hook_types',
@@ -358,11 +371,6 @@ def check(self, dct: dict[str, Any]) -> None:
     cfgv.Optional('files', check_string_regex, ''),
     cfgv.Optional('exclude', check_string_regex, '^$'),
     cfgv.Optional('fail_fast', cfgv.check_bool, False),
-    cfgv.Optional(
-        'minimum_pre_commit_version',
-        cfgv.check_and(cfgv.check_string, check_min_version),
-        '0',
-    ),
     cfgv.WarnAdditionalKeys(
         (
             'repos',

pre_commit/repository.py

@@ -12,7 +12,6 @@
 from pre_commit.clientlib import load_manifest
 from pre_commit.clientlib import LOCAL
 from pre_commit.clientlib import META
-from pre_commit.clientlib import parse_version
 from pre_commit.hook import Hook
 from pre_commit.lang_base import environment_dir
 from pre_commit.prefix import Prefix
@@ -124,15 +123,6 @@ def _hook(
     for dct in rest:
         ret.update(dct)
 
-    version = ret['minimum_pre_commit_version']
-    if parse_version(version) > parse_version(C.VERSION):
-        logger.error(
-            f'The hook `{ret["id"]}` requires pre-commit version {version} '
-            f'but version {C.VERSION} is installed.  '
-            f'Perhaps run `pip install --upgrade pre-commit`.',
-        )
-        exit(1)
-
     lang = ret['language']
     if ret['language_version'] == C.DEFAULT:
         ret['language_version'] = root_config['default_language_version'][lang]

tests/clientlib_test.py

@@ -40,56 +40,51 @@ def test_check_type_tag_success():
 
 
 @pytest.mark.parametrize(
-    ('config_obj', 'expected'), (
-        (
-            {
-                'repos': [{
-                    'repo': 'git@github.com:pre-commit/pre-commit-hooks',
-                    'rev': 'cd74dc150c142c3be70b24eaf0b02cae9d235f37',
-                    'hooks': [{'id': 'pyflakes', 'files': '\\.py$'}],
-                }],
-            },
-            True,
-        ),
-        (
-            {
-                'repos': [{
-                    'repo': 'git@github.com:pre-commit/pre-commit-hooks',
-                    'rev': 'cd74dc150c142c3be70b24eaf0b02cae9d235f37',
-                    'hooks': [
-                        {
-                            'id': 'pyflakes',
-                            'files': '\\.py$',
-                            'args': ['foo', 'bar', 'baz'],
-                        },
-                    ],
-                }],
-            },
-            True,
-        ),
-        (
-            {
-                'repos': [{
-                    'repo': 'git@github.com:pre-commit/pre-commit-hooks',
-                    'rev': 'cd74dc150c142c3be70b24eaf0b02cae9d235f37',
-                    'hooks': [
-                        {
-                            'id': 'pyflakes',
-                            'files': '\\.py$',
-                            # Exclude pattern must be a string
-                            'exclude': 0,
-                            'args': ['foo', 'bar', 'baz'],
-                        },
-                    ],
-                }],
-            },
-            False,
-        ),
+    'cfg',
+    (
+        {
+            'repos': [{
+                'repo': 'git@github.com:pre-commit/pre-commit-hooks',
+                'rev': 'cd74dc150c142c3be70b24eaf0b02cae9d235f37',
+                'hooks': [{'id': 'pyflakes', 'files': '\\.py$'}],
+            }],
+        },
+        {
+            'repos': [{
+                'repo': 'git@github.com:pre-commit/pre-commit-hooks',
+                'rev': 'cd74dc150c142c3be70b24eaf0b02cae9d235f37',
+                'hooks': [
+                    {
+                        'id': 'pyflakes',
+                        'files': '\\.py$',
+                        'args': ['foo', 'bar', 'baz'],
+                    },
+                ],
+            }],
+        },
     ),
 )
-def test_config_valid(config_obj, expected):
-    ret = is_valid_according_to_schema(config_obj, CONFIG_SCHEMA)
-    assert ret is expected
+def test_config_valid(cfg):
+    assert is_valid_according_to_schema(cfg, CONFIG_SCHEMA)
+
+
+def test_invalid_config_wrong_type():
+    cfg = {
+        'repos': [{
+            'repo': 'git@github.com:pre-commit/pre-commit-hooks',
+            'rev': 'cd74dc150c142c3be70b24eaf0b02cae9d235f37',
+            'hooks': [
+                {
+                    'id': 'pyflakes',
+                    'files': '\\.py$',
+                    # Exclude pattern must be a string
+                    'exclude': 0,
+                    'args': ['foo', 'bar', 'baz'],
+                },
+            ],
+        }],
+    }
+    assert not is_valid_according_to_schema(cfg, CONFIG_SCHEMA)
 
 
 def test_local_hooks_with_rev_fails():
@@ -198,14 +193,13 @@ def test_warn_mutable_rev_conditional():
     ),
 )
 def test_sensible_regex_validators_dont_pass_none(validator_cls):
-    key = 'files'
+    validator = validator_cls('files', cfgv.check_string)
     with pytest.raises(cfgv.ValidationError) as excinfo:
-        validator = validator_cls(key, cfgv.check_string)
-        validator.check({key: None})
+        validator.check({'files': None})
 
     assert str(excinfo.value) == (
         '\n'
-        f'==> At key: {key}'
+        '==> At key: files'
         '\n'
         '=====> Expected string got NoneType'
     )
@@ -298,46 +292,36 @@ def test_validate_optional_sensible_regex_at_top_level(caplog, regex, warning):
 
 
 @pytest.mark.parametrize(
-    ('manifest_obj', 'expected'),
+    'manifest_obj',
     (
-        (
-            [{
-                'id': 'a',
-                'name': 'b',
-                'entry': 'c',
-                'language': 'python',
-                'files': r'\.py$',
-            }],
-            True,
-        ),
-        (
-            [{
-                'id': 'a',
-                'name': 'b',
-                'entry': 'c',
-                'language': 'python',
-                'language_version': 'python3.4',
-                'files': r'\.py$',
-            }],
-            True,
-        ),
-        (
-            # A regression in 0.13.5: always_run and files are permissible
-            [{
-                'id': 'a',
-                'name': 'b',
-                'entry': 'c',
-                'language': 'python',
-                'files': '',
-                'always_run': True,
-            }],
-            True,
-        ),
+        [{
+            '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$',
+        }],
+        # A regression in 0.13.5: always_run and files are permissible
+        [{
+            'id': 'a',
+            'name': 'b',
+            'entry': 'c',
+            'language': 'python',
+            'files': '',
+            'always_run': True,
+        }],
     ),
 )
-def test_valid_manifests(manifest_obj, expected):
-    ret = is_valid_according_to_schema(manifest_obj, MANIFEST_SCHEMA)
-    assert ret is expected
+def test_valid_manifests(manifest_obj):
+    assert is_valid_according_to_schema(manifest_obj, MANIFEST_SCHEMA)
 
 
 @pytest.mark.parametrize(
@@ -393,8 +377,39 @@ def test_parse_version():
 
 
 def test_minimum_pre_commit_version_failing():
+    cfg = {'repos': [], 'minimum_pre_commit_version': '999'}
+    with pytest.raises(cfgv.ValidationError) as excinfo:
+        cfgv.validate(cfg, CONFIG_SCHEMA)
+    assert str(excinfo.value) == (
+        f'\n'
+        f'==> At Config()\n'
+        f'==> At key: minimum_pre_commit_version\n'
+        f'=====> pre-commit version 999 is required but version {C.VERSION} '
+        f'is installed.  Perhaps run `pip install --upgrade pre-commit`.'
+    )
+
+
+def test_minimum_pre_commit_version_failing_in_config():
+    cfg = {'repos': [sample_local_config()]}
+    cfg['repos'][0]['hooks'][0]['minimum_pre_commit_version'] = '999'
+    with pytest.raises(cfgv.ValidationError) as excinfo:
+        cfgv.validate(cfg, CONFIG_SCHEMA)
+    assert str(excinfo.value) == (
+        f'\n'
+        f'==> At Config()\n'
+        f'==> At key: repos\n'
+        f"==> At Repository(repo='local')\n"
+        f'==> At key: hooks\n'
+        f"==> At Hook(id='do_not_commit')\n"
+        f'==> At key: minimum_pre_commit_version\n'
+        f'=====> pre-commit version 999 is required but version {C.VERSION} '
+        f'is installed.  Perhaps run `pip install --upgrade pre-commit`.'
+    )
+
+
+def test_minimum_pre_commit_version_failing_before_other_error():
+    cfg = {'repos': 5, 'minimum_pre_commit_version': '999'}
     with pytest.raises(cfgv.ValidationError) as excinfo:
-        cfg = {'repos': [], 'minimum_pre_commit_version': '999'}
         cfgv.validate(cfg, CONFIG_SCHEMA)
     assert str(excinfo.value) == (
         f'\n'

tests/repository_test.py

@@ -9,7 +9,6 @@
 
 import cfgv
 import pytest
-import re_assert
 
 import pre_commit.constants as C
 from pre_commit import lang_base
@@ -27,7 +26,6 @@
 from pre_commit.util import cmd_output_b
 from testing.fixtures import make_config_from_repo
 from testing.fixtures import make_repo
-from testing.fixtures import modify_manifest
 from testing.language_helpers import run_language
 from testing.util import cwd
 from testing.util import get_resource_path
@@ -433,32 +431,6 @@ def test_hook_id_not_present(tempdir_factory, store, caplog):
     )
 
 
-def test_too_new_version(tempdir_factory, store, caplog):
-    path = make_repo(tempdir_factory, 'script_hooks_repo')
-    with modify_manifest(path) as manifest:
-        manifest[0]['minimum_pre_commit_version'] = '999.0.0'
-    config = make_config_from_repo(path)
-    with pytest.raises(SystemExit):
-        _get_hook(config, store, 'bash_hook')
-    _, msg = caplog.messages
-    pattern = re_assert.Matches(
-        r'^The hook `bash_hook` requires pre-commit version 999\.0\.0 but '
-        r'version \d+\.\d+\.\d+ is installed.  '
-        r'Perhaps run `pip install --upgrade pre-commit`\.$',
-    )
-    pattern.assert_matches(msg)
-
-
-@pytest.mark.parametrize('version', ('0.1.0', C.VERSION))
-def test_versions_ok(tempdir_factory, store, version):
-    path = make_repo(tempdir_factory, 'script_hooks_repo')
-    with modify_manifest(path) as manifest:
-        manifest[0]['minimum_pre_commit_version'] = version
-    config = make_config_from_repo(path)
-    # Should succeed
-    _get_hook(config, store, 'bash_hook')
-
-
 def test_manifest_hooks(tempdir_factory, store):
     path = make_repo(tempdir_factory, 'script_hooks_repo')
     config = make_config_from_repo(path)