From 047439abffb164edd5b49e50439fd63a625be3da Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sat, 9 Dec 2023 15:34:16 -0500 Subject: [PATCH] attempt minimum_pre_commit_version first when parsing configs --- pre_commit/clientlib.py | 20 ++-- pre_commit/repository.py | 10 -- tests/clientlib_test.py | 195 +++++++++++++++++++++------------------ tests/repository_test.py | 28 ------ 4 files changed, 119 insertions(+), 134 deletions(-) diff --git a/pre_commit/clientlib.py b/pre_commit/clientlib.py index 9f41bf4b..a49465e8 100644 --- a/pre_commit/clientlib.py +++ b/pre_commit/clientlib.py @@ -102,6 +102,13 @@ class StagesMigration(StagesMigrationNoDefault): 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 @@ MANIFEST_HOOK_DICT = cfgv.Map( 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 @@ DEFAULT_LANGUAGE_VERSION = cfgv.Map( 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 @@ CONFIG_SCHEMA = cfgv.Map( 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', diff --git a/pre_commit/repository.py b/pre_commit/repository.py index 439a09b4..aa841856 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -12,7 +12,6 @@ from pre_commit.all_languages import languages 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] diff --git a/tests/clientlib_test.py b/tests/clientlib_test.py index 568b2e97..eaa8a044 100644 --- a/tests/clientlib_test.py +++ b/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' diff --git a/tests/repository_test.py b/tests/repository_test.py index b8dde99b..ac065ec4 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -9,7 +9,6 @@ from unittest import mock 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 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)