Skip to content

Commit f2661bf

Browse files
asottileasottile
authored
Merge pull request #2808 from pre-commit/hook-stage-and-type-match
make --hook-type and stages match
2 parents 02e9680 + e3e17a1 commit f2661bf

File tree

10 files changed

+147
-42
lines changed

10 files changed

+147
-42
lines changed

‎pre_commit/clientlib.py‎

Lines changed: 60 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import shlex
77
import sys
88
from typing import Any
9+
from typing import NamedTuple
910
from typing import Sequence
1011

1112
import cfgv
@@ -20,6 +21,20 @@
2021

2122
check_string_regex = cfgv.check_and(cfgv.check_string, cfgv.check_regex)
2223

24+
HOOK_TYPES = (
25+
'commit-msg',
26+
'post-checkout',
27+
'post-commit',
28+
'post-merge',
29+
'post-rewrite',
30+
'pre-commit',
31+
'pre-merge-commit',
32+
'pre-push',
33+
'prepare-commit-msg',
34+
)
35+
# `manual` is not invoked by any installed git hook. See #719
36+
STAGES = (*HOOK_TYPES, 'manual')
37+
2338

2439
def check_type_tag(tag: str) -> None:
2540
if tag not in ALL_TAGS:
@@ -43,6 +58,46 @@ def check_min_version(version: str) -> None:
4358
)
4459

4560

61+
_STAGES = {
62+
'commit': 'pre-commit',
63+
'merge-commit': 'pre-merge-commit',
64+
'push': 'pre-push',
65+
}
66+
67+
68+
def transform_stage(stage: str) -> str:
69+
return _STAGES.get(stage, stage)
70+
71+
72+
class StagesMigrationNoDefault(NamedTuple):
73+
key: str
74+
default: Sequence[str]
75+
76+
def check(self, dct: dict[str, Any]) -> None:
77+
if self.key not in dct:
78+
return
79+
80+
val = dct[self.key]
81+
cfgv.check_array(cfgv.check_any)(val)
82+
83+
val = [transform_stage(v) for v in val]
84+
cfgv.check_array(cfgv.check_one_of(STAGES))(val)
85+
86+
def apply_default(self, dct: dict[str, Any]) -> None:
87+
if self.key not in dct:
88+
return
89+
dct[self.key] = [transform_stage(v) for v in dct[self.key]]
90+
91+
def remove_default(self, dct: dict[str, Any]) -> None:
92+
raise NotImplementedError
93+
94+
95+
class StagesMigration(StagesMigrationNoDefault):
96+
def apply_default(self, dct: dict[str, Any]) -> None:
97+
dct.setdefault(self.key, self.default)
98+
super().apply_default(dct)
99+
100+
46101
MANIFEST_HOOK_DICT = cfgv.Map(
47102
'Hook', 'id',
48103

@@ -70,7 +125,7 @@ def check_min_version(version: str) -> None:
70125
cfgv.Optional('log_file', cfgv.check_string, ''),
71126
cfgv.Optional('minimum_pre_commit_version', cfgv.check_string, '0'),
72127
cfgv.Optional('require_serial', cfgv.check_bool, False),
73-
cfgv.Optional('stages', cfgv.check_array(cfgv.check_one_of(C.STAGES)), []),
128+
StagesMigration('stages', []),
74129
cfgv.Optional('verbose', cfgv.check_bool, False),
75130
)
76131
MANIFEST_SCHEMA = cfgv.Array(MANIFEST_HOOK_DICT)
@@ -241,7 +296,9 @@ def check(self, dct: dict[str, Any]) -> None:
241296
cfgv.OptionalNoDefault(item.key, item.check_fn)
242297
for item in MANIFEST_HOOK_DICT.items
243298
if item.key != 'id'
299+
if item.key != 'stages'
244300
),
301+
StagesMigrationNoDefault('stages', []),
245302
OptionalSensibleRegexAtHook('files', cfgv.check_string),
246303
OptionalSensibleRegexAtHook('exclude', cfgv.check_string),
247304
)
@@ -290,17 +347,13 @@ def check(self, dct: dict[str, Any]) -> None:
290347
cfgv.RequiredRecurse('repos', cfgv.Array(CONFIG_REPO_DICT)),
291348
cfgv.Optional(
292349
'default_install_hook_types',
293-
cfgv.check_array(cfgv.check_one_of(C.HOOK_TYPES)),
350+
cfgv.check_array(cfgv.check_one_of(HOOK_TYPES)),
294351
['pre-commit'],
295352
),
296353
cfgv.OptionalRecurse(
297354
'default_language_version', DEFAULT_LANGUAGE_VERSION, {},
298355
),
299-
cfgv.Optional(
300-
'default_stages',
301-
cfgv.check_array(cfgv.check_one_of(C.STAGES)),
302-
C.STAGES,
303-
),
356+
StagesMigration('default_stages', STAGES),
304357
cfgv.Optional('files', check_string_regex, ''),
305358
cfgv.Optional('exclude', check_string_regex, '^$'),
306359
cfgv.Optional('fail_fast', cfgv.check_bool, False),

‎pre_commit/commands/hook_impl.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ def _ns(
8484
) -> argparse.Namespace:
8585
return argparse.Namespace(
8686
color=color,
87-
hook_stage=hook_type.replace('pre-', ''),
87+
hook_stage=hook_type,
8888
remote_branch=remote_branch,
8989
local_branch=local_branch,
9090
from_ref=from_ref,

‎pre_commit/constants.py‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,17 +10,4 @@
1010

1111
VERSION = importlib.metadata.version('pre_commit')
1212

13-
# `manual` is not invoked by any installed git hook. See #719
14-
STAGES = (
15-
'commit', 'merge-commit', 'prepare-commit-msg', 'commit-msg',
16-
'post-commit', 'manual', 'post-checkout', 'push', 'post-merge',
17-
'post-rewrite',
18-
)
19-
20-
HOOK_TYPES = (
21-
'pre-commit', 'pre-merge-commit', 'pre-push', 'prepare-commit-msg',
22-
'commit-msg', 'post-commit', 'post-checkout', 'post-merge',
23-
'post-rewrite',
24-
)
25-
2613
DEFAULT = 'default'

‎pre_commit/main.py‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
from typing import Sequence
88

99
import pre_commit.constants as C
10+
from pre_commit import clientlib
1011
from pre_commit import git
1112
from pre_commit.color import add_color_option
1213
from pre_commit.commands.autoupdate import autoupdate
@@ -52,7 +53,7 @@ def _add_config_option(parser: argparse.ArgumentParser) -> None:
5253
def _add_hook_type_option(parser: argparse.ArgumentParser) -> None:
5354
parser.add_argument(
5455
'-t', '--hook-type',
55-
choices=C.HOOK_TYPES, action='append', dest='hook_types',
56+
choices=clientlib.HOOK_TYPES, action='append', dest='hook_types',
5657
)
5758

5859

@@ -73,7 +74,10 @@ def _add_run_options(parser: argparse.ArgumentParser) -> None:
7374
help='When hooks fail, run `git diff` directly afterward.',
7475
)
7576
parser.add_argument(
76-
'--hook-stage', choices=C.STAGES, default='commit',
77+
'--hook-stage',
78+
choices=clientlib.STAGES,
79+
type=clientlib.transform_stage,
80+
default='pre-commit',
7781
help='The stage during which the hook is fired. One of %(choices)s',
7882
)
7983
parser.add_argument(

‎testing/util.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ def run_opts(
4646
to_ref='',
4747
remote_name='',
4848
remote_url='',
49-
hook_stage='commit',
49+
hook_stage='pre-commit',
5050
show_diff_on_failure=False,
5151
commit_msg_filename='',
5252
prepare_commit_message_source='',

‎tests/clientlib_test.py‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
from pre_commit.clientlib import CONFIG_REPO_DICT
1313
from pre_commit.clientlib import CONFIG_SCHEMA
1414
from pre_commit.clientlib import DEFAULT_LANGUAGE_VERSION
15+
from pre_commit.clientlib import MANIFEST_HOOK_DICT
1516
from pre_commit.clientlib import MANIFEST_SCHEMA
1617
from pre_commit.clientlib import META_HOOK_DICT
1718
from pre_commit.clientlib import OptionalSensibleRegexAtHook
@@ -416,3 +417,50 @@ def test_warn_additional(schema):
416417
x for x in schema.items if isinstance(x, cfgv.WarnAdditionalKeys)
417418
)
418419
assert allowed_keys == set(warn_additional.keys)
420+
421+
422+
def test_stages_migration_for_default_stages():
423+
cfg = {
424+
'default_stages': ['commit-msg', 'push', 'commit', 'merge-commit'],
425+
'repos': [],
426+
}
427+
cfgv.validate(cfg, CONFIG_SCHEMA)
428+
cfg = cfgv.apply_defaults(cfg, CONFIG_SCHEMA)
429+
assert cfg['default_stages'] == [
430+
'commit-msg', 'pre-push', 'pre-commit', 'pre-merge-commit',
431+
]
432+
433+
434+
def test_manifest_stages_defaulting():
435+
dct = {
436+
'id': 'fake-hook',
437+
'name': 'fake-hook',
438+
'entry': 'fake-hook',
439+
'language': 'system',
440+
'stages': ['commit-msg', 'push', 'commit', 'merge-commit'],
441+
}
442+
cfgv.validate(dct, MANIFEST_HOOK_DICT)
443+
dct = cfgv.apply_defaults(dct, MANIFEST_HOOK_DICT)
444+
assert dct['stages'] == [
445+
'commit-msg', 'pre-push', 'pre-commit', 'pre-merge-commit',
446+
]
447+
448+
449+
def test_config_hook_stages_defaulting_missing():
450+
dct = {'id': 'fake-hook'}
451+
cfgv.validate(dct, CONFIG_HOOK_DICT)
452+
dct = cfgv.apply_defaults(dct, CONFIG_HOOK_DICT)
453+
assert dct == {'id': 'fake-hook'}
454+
455+
456+
def test_config_hook_stages_defaulting():
457+
dct = {
458+
'id': 'fake-hook',
459+
'stages': ['commit-msg', 'push', 'commit', 'merge-commit'],
460+
}
461+
cfgv.validate(dct, CONFIG_HOOK_DICT)
462+
dct = cfgv.apply_defaults(dct, CONFIG_HOOK_DICT)
463+
assert dct == {
464+
'id': 'fake-hook',
465+
'stages': ['commit-msg', 'pre-push', 'pre-commit', 'pre-merge-commit'],
466+
}

‎tests/commands/hook_impl_test.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ def test_check_args_length_prepare_commit_msg_error():
142142
def test_run_ns_pre_commit():
143143
ns = hook_impl._run_ns('pre-commit', True, (), b'')
144144
assert ns is not None
145-
assert ns.hook_stage == 'commit'
145+
assert ns.hook_stage == 'pre-commit'
146146
assert ns.color is True
147147

148148

@@ -245,7 +245,7 @@ def test_run_ns_pre_push_updating_branch(push_example):
245245
ns = hook_impl._run_ns('pre-push', False, args, stdin)
246246

247247
assert ns is not None
248-
assert ns.hook_stage == 'push'
248+
assert ns.hook_stage == 'pre-push'
249249
assert ns.color is False
250250
assert ns.remote_name == 'origin'
251251
assert ns.remote_url == src

‎tests/commands/run_test.py‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -354,13 +354,13 @@ def test_show_diff_on_failure(
354354
({'hook': 'bash_hook'}, (b'Bash hook', b'Passed'), 0, True),
355355
(
356356
{'hook': 'nope'},
357-
(b'No hook with id `nope` in stage `commit`',),
357+
(b'No hook with id `nope` in stage `pre-commit`',),
358358
1,
359359
True,
360360
),
361361
(
362-
{'hook': 'nope', 'hook_stage': 'push'},
363-
(b'No hook with id `nope` in stage `push`',),
362+
{'hook': 'nope', 'hook_stage': 'pre-push'},
363+
(b'No hook with id `nope` in stage `pre-push`',),
364364
1,
365365
True,
366366
),
@@ -818,7 +818,7 @@ def test_stages(cap_out, store, repo_with_passing_hook):
818818
'language': 'pygrep',
819819
'stages': [stage],
820820
}
821-
for i, stage in enumerate(('commit', 'push', 'manual'), 1)
821+
for i, stage in enumerate(('pre-commit', 'pre-push', 'manual'), 1)
822822
],
823823
}
824824
add_config_to_repo(repo_with_passing_hook, config)
@@ -833,8 +833,8 @@ def _run_for_stage(stage):
833833
assert printed.count(b'hook ') == 1
834834
return printed
835835

836-
assert _run_for_stage('commit').startswith(b'hook 1...')
837-
assert _run_for_stage('push').startswith(b'hook 2...')
836+
assert _run_for_stage('pre-commit').startswith(b'hook 1...')
837+
assert _run_for_stage('pre-push').startswith(b'hook 2...')
838838
assert _run_for_stage('manual').startswith(b'hook 3...')
839839

840840

@@ -1173,7 +1173,7 @@ def test_args_hook_only(cap_out, store, repo_with_passing_hook):
11731173
),
11741174
'language': 'system',
11751175
'files': r'\.py$',
1176-
'stages': ['commit'],
1176+
'stages': ['pre-commit'],
11771177
},
11781178
{
11791179
'id': 'do_not_commit',

‎tests/main_test.py‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,3 +216,9 @@ def test_expected_fatal_error_no_git_repo(in_tmpdir, cap_out, mock_store_dir):
216216
'Is it installed, and are you in a Git repository directory?'
217217
)
218218
assert cap_out_lines[-1] == f'Check the log at {log_file}'
219+
220+
221+
def test_hook_stage_migration(mock_store_dir):
222+
with mock.patch.object(main, 'run') as mck:
223+
main.main(('run', '--hook-stage', 'commit'))
224+
assert mck.call_args[0][2].hook_stage == 'pre-commit'

‎tests/repository_test.py‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,7 @@ def test_local_python_repo(store, local_python_config):
417417
def test_default_language_version(store, local_python_config):
418418
config: dict[str, Any] = {
419419
'default_language_version': {'python': 'fake'},
420-
'default_stages': ['commit'],
420+
'default_stages': ['pre-commit'],
421421
'repos': [local_python_config],
422422
}
423423

@@ -434,18 +434,18 @@ def test_default_language_version(store, local_python_config):
434434
def test_default_stages(store, local_python_config):
435435
config: dict[str, Any] = {
436436
'default_language_version': {'python': C.DEFAULT},
437-
'default_stages': ['commit'],
437+
'default_stages': ['pre-commit'],
438438
'repos': [local_python_config],
439439
}
440440

441441
# `stages` was not set, should default
442442
hook, = all_hooks(config, store)
443-
assert hook.stages == ['commit']
443+
assert hook.stages == ['pre-commit']
444444

445445
# `stages` is set, should not default
446-
config['repos'][0]['hooks'][0]['stages'] = ['push']
446+
config['repos'][0]['hooks'][0]['stages'] = ['pre-push']
447447
hook, = all_hooks(config, store)
448-
assert hook.stages == ['push']
448+
assert hook.stages == ['pre-push']
449449

450450

451451
def test_hook_id_not_present(tempdir_factory, store, caplog):
@@ -513,11 +513,18 @@ def test_manifest_hooks(tempdir_factory, store):
513513
name='Bash hook',
514514
pass_filenames=True,
515515
require_serial=False,
516-
stages=(
517-
'commit', 'merge-commit', 'prepare-commit-msg', 'commit-msg',
518-
'post-commit', 'manual', 'post-checkout', 'push', 'post-merge',
516+
stages=[
517+
'commit-msg',
518+
'post-checkout',
519+
'post-commit',
520+
'post-merge',
519521
'post-rewrite',
520-
),
522+
'pre-commit',
523+
'pre-merge-commit',
524+
'pre-push',
525+
'prepare-commit-msg',
526+
'manual',
527+
],
521528
types=['file'],
522529
types_or=[],
523530
verbose=False,

0 commit comments

Comments
 (0)