Merge pull request #2322 from pre-commit/default-install-hook-types

asottile · web-flow · commit 7602abc3cfa0 · 2022-04-02T15:36:59.000-04:00
implement default_install_hook_types
diff --git a/pre_commit/clientlib.py b/pre_commit/clientlib.py
@@ -336,6 +336,11 @@ def check(self, dct: dict[str, Any]) -> None:
     'Config', None,
 
     cfgv.RequiredRecurse('repos', cfgv.Array(CONFIG_REPO_DICT)),
+    cfgv.Optional(
+        'default_install_hook_types',
+        cfgv.check_array(cfgv.check_one_of(C.HOOK_TYPES)),
+        ['pre-commit'],
+    ),
     cfgv.OptionalRecurse(
         'default_language_version', DEFAULT_LANGUAGE_VERSION, {},
     ),
@@ -355,6 +360,7 @@ def check(self, dct: dict[str, Any]) -> None:
     cfgv.WarnAdditionalKeys(
         (
             'repos',
+            'default_install_hook_types',
             'default_language_version',
             'default_stages',
             'files',
diff --git a/pre_commit/commands/init_templatedir.py b/pre_commit/commands/init_templatedir.py
@@ -2,7 +2,6 @@
 
 import logging
 import os.path
-from typing import Sequence
 
 from pre_commit.commands.install_uninstall import install
 from pre_commit.store import Store
@@ -16,7 +15,7 @@ def init_templatedir(
         config_file: str,
         store: Store,
         directory: str,
-        hook_types: Sequence[str],
+        hook_types: list[str] | None,
         skip_on_missing_config: bool = True,
 ) -> int:
     install(
diff --git a/pre_commit/commands/install_uninstall.py b/pre_commit/commands/install_uninstall.py
@@ -5,10 +5,10 @@
 import shlex
 import shutil
 import sys
-from typing import Sequence
 
 from pre_commit import git
 from pre_commit import output
+from pre_commit.clientlib import InvalidConfigError
 from pre_commit.clientlib import load_config
 from pre_commit.repository import all_hooks
 from pre_commit.repository import install_hook_envs
@@ -32,6 +32,18 @@
 TEMPLATE_END = '# end templated\n'
 
 
+def _hook_types(cfg_filename: str, hook_types: list[str] | None) -> list[str]:
+    if hook_types is not None:
+        return hook_types
+    else:
+        try:
+            cfg = load_config(cfg_filename)
+        except InvalidConfigError:
+            return ['pre-commit']
+        else:
+            return cfg['default_install_hook_types']
+
+
 def _hook_paths(
         hook_type: str,
         git_dir: str | None = None,
@@ -103,7 +115,7 @@ def _install_hook_script(
 def install(
         config_file: str,
         store: Store,
-        hook_types: Sequence[str],
+        hook_types: list[str] | None,
         overwrite: bool = False,
         hooks: bool = False,
         skip_on_missing_config: bool = False,
@@ -116,7 +128,7 @@ def install(
         )
         return 1
 
-    for hook_type in hook_types:
+    for hook_type in _hook_types(config_file, hook_types):
         _install_hook_script(
             config_file, hook_type,
             overwrite=overwrite,
@@ -150,7 +162,7 @@ def _uninstall_hook_script(hook_type: str) -> None:
         output.write_line(f'Restored previous hooks to {hook_path}')
 
 
-def uninstall(hook_types: Sequence[str]) -> int:
-    for hook_type in hook_types:
+def uninstall(config_file: str, hook_types: list[str] | None) -> int:
+    for hook_type in _hook_types(config_file, hook_types):
         _uninstall_hook_script(hook_type)
     return 0
diff --git a/pre_commit/constants.py b/pre_commit/constants.py
@@ -24,4 +24,10 @@
     'post-rewrite',
 )
 
+HOOK_TYPES = (
+    'pre-commit', 'pre-merge-commit', 'pre-push', 'prepare-commit-msg',
+    'commit-msg', 'post-commit', 'post-checkout', 'post-merge',
+    'post-rewrite',
+)
+
 DEFAULT = 'default'
diff --git a/pre_commit/main.py b/pre_commit/main.py
@@ -4,7 +4,6 @@
 import logging
 import os
 import sys
-from typing import Any
 from typing import Sequence
 
 import pre_commit.constants as C
@@ -46,34 +45,10 @@ def _add_config_option(parser: argparse.ArgumentParser) -> None:
     )
 
 
-class AppendReplaceDefault(argparse.Action):
-    def __init__(self, *args: Any, **kwargs: Any) -> None:
-        super().__init__(*args, **kwargs)
-        self.appended = False
-
-    def __call__(
-            self,
-            parser: argparse.ArgumentParser,
-            namespace: argparse.Namespace,
-            values: str | Sequence[str] | None,
-            option_string: str | None = None,
-    ) -> None:
-        if not self.appended:
-            setattr(namespace, self.dest, [])
-            self.appended = True
-        getattr(namespace, self.dest).append(values)
-
-
 def _add_hook_type_option(parser: argparse.ArgumentParser) -> None:
     parser.add_argument(
-        '-t', '--hook-type', choices=(
-            'pre-commit', 'pre-merge-commit', 'pre-push', 'prepare-commit-msg',
-            'commit-msg', 'post-commit', 'post-checkout', 'post-merge',
-            'post-rewrite',
-        ),
-        action=AppendReplaceDefault,
-        default=['pre-commit'],
-        dest='hook_types',
+        '-t', '--hook-type',
+        choices=C.HOOK_TYPES, action='append', dest='hook_types',
     )
 
 
@@ -399,7 +374,10 @@ def main(argv: Sequence[str] | None = None) -> int:
         elif args.command == 'try-repo':
             return try_repo(args)
         elif args.command == 'uninstall':
-            return uninstall(hook_types=args.hook_types)
+            return uninstall(
+                config_file=args.config,
+                hook_types=args.hook_types,
+            )
         else:
             raise NotImplementedError(
                 f'Command {args.command} not implemented.',
diff --git a/tests/commands/install_uninstall_test.py b/tests/commands/install_uninstall_test.py
@@ -7,6 +7,7 @@
 
 import pre_commit.constants as C
 from pre_commit import git
+from pre_commit.commands.install_uninstall import _hook_types
 from pre_commit.commands.install_uninstall import CURRENT_HASH
 from pre_commit.commands.install_uninstall import install
 from pre_commit.commands.install_uninstall import install_hooks
@@ -27,6 +28,36 @@
 from testing.util import git_commit
 
 
+def test_hook_types_explicitly_listed():
+    assert _hook_types(os.devnull, ['pre-push']) == ['pre-push']
+
+
+def test_hook_types_default_value_when_not_specified():
+    assert _hook_types(os.devnull, None) == ['pre-commit']
+
+
+def test_hook_types_configured(tmpdir):
+    cfg = tmpdir.join('t.cfg')
+    cfg.write('default_install_hook_types: [pre-push]\nrepos: []\n')
+
+    assert _hook_types(str(cfg), None) == ['pre-push']
+
+
+def test_hook_types_configured_nonsense(tmpdir):
+    cfg = tmpdir.join('t.cfg')
+    cfg.write('default_install_hook_types: []\nrepos: []\n')
+
+    # hopefully the user doesn't do this, but the code allows it!
+    assert _hook_types(str(cfg), None) == []
+
+
+def test_hook_types_configuration_has_error(tmpdir):
+    cfg = tmpdir.join('t.cfg')
+    cfg.write('[')
+
+    assert _hook_types(str(cfg), None) == ['pre-commit']
+
+
 def test_is_not_script():
     assert is_our_script('setup.py') is False
 
@@ -61,7 +92,7 @@ def test_install_multiple_hooks_at_once(in_git_dir, store):
     install(C.CONFIG_FILE, store, hook_types=['pre-commit', 'pre-push'])
     assert in_git_dir.join('.git/hooks/pre-commit').exists()
     assert in_git_dir.join('.git/hooks/pre-push').exists()
-    uninstall(hook_types=['pre-commit', 'pre-push'])
+    uninstall(C.CONFIG_FILE, hook_types=['pre-commit', 'pre-push'])
     assert not in_git_dir.join('.git/hooks/pre-commit').exists()
     assert not in_git_dir.join('.git/hooks/pre-push').exists()
 
@@ -79,14 +110,14 @@ def test_install_hooks_dead_symlink(in_git_dir, store):
 
 
 def test_uninstall_does_not_blow_up_when_not_there(in_git_dir):
-    assert uninstall(hook_types=['pre-commit']) == 0
+    assert uninstall(C.CONFIG_FILE, hook_types=['pre-commit']) == 0
 
 
 def test_uninstall(in_git_dir, store):
     assert not in_git_dir.join('.git/hooks/pre-commit').exists()
     install(C.CONFIG_FILE, store, hook_types=['pre-commit'])
     assert in_git_dir.join('.git/hooks/pre-commit').exists()
-    uninstall(hook_types=['pre-commit'])
+    uninstall(C.CONFIG_FILE, hook_types=['pre-commit'])
     assert not in_git_dir.join('.git/hooks/pre-commit').exists()
 
 
@@ -416,7 +447,7 @@ def test_uninstall_restores_legacy_hooks(tempdir_factory, store):
 
         # Now install and uninstall pre-commit
         assert install(C.CONFIG_FILE, store, hook_types=['pre-commit']) == 0
-        assert uninstall(hook_types=['pre-commit']) == 0
+        assert uninstall(C.CONFIG_FILE, hook_types=['pre-commit']) == 0
 
         # Make sure we installed the "old" hook correctly
         ret, output = _get_commit_output(tempdir_factory, touch_file='baz')
@@ -451,7 +482,7 @@ def test_uninstall_doesnt_remove_not_our_hooks(in_git_dir):
     pre_commit.write('#!/usr/bin/env bash\necho 1\n')
     make_executable(pre_commit.strpath)
 
-    assert uninstall(hook_types=['pre-commit']) == 0
+    assert uninstall(C.CONFIG_FILE, hook_types=['pre-commit']) == 0
 
     assert pre_commit.exists()
 
@@ -1007,3 +1038,16 @@ def test_install_temporarily_allow_mising_config(tempdir_factory, store):
             'Skipping `pre-commit`.'
         )
         assert expected in output
+
+
+def test_install_uninstall_default_hook_types(in_git_dir, store):
+    cfg_src = 'default_install_hook_types: [pre-commit, pre-push]\nrepos: []\n'
+    in_git_dir.join(C.CONFIG_FILE).write(cfg_src)
+
+    assert not install(C.CONFIG_FILE, store, hook_types=None)
+    assert os.access(in_git_dir.join('.git/hooks/pre-commit').strpath, os.X_OK)
+    assert os.access(in_git_dir.join('.git/hooks/pre-push').strpath, os.X_OK)
+
+    assert not uninstall(C.CONFIG_FILE, hook_types=None)
+    assert not in_git_dir.join('.git/hooks/pre-commit').exists()
+    assert not in_git_dir.join('.git/hooks/pre-push').exists()
diff --git a/tests/main_test.py b/tests/main_test.py
@@ -14,20 +14,6 @@
 from testing.util import cwd
 
 
-@pytest.mark.parametrize(
-    ('argv', 'expected'),
-    (
-        ((), ['f']),
-        (('--f', 'x'), ['x']),
-        (('--f', 'x', '--f', 'y'), ['x', 'y']),
-    ),
-)
-def test_append_replace_default(argv, expected):
-    parser = argparse.ArgumentParser()
-    parser.add_argument('--f', action=main.AppendReplaceDefault, default=['f'])
-    assert parser.parse_args(argv).f == expected
-
-
 def _args(**kwargs):
     kwargs.setdefault('command', 'help')
     kwargs.setdefault('config', C.CONFIG_FILE)
@@ -172,7 +158,7 @@ def test_init_templatedir(mock_store_dir):
 
     assert patch.call_count == 1
     assert 'tdir' in patch.call_args[0]
-    assert patch.call_args[1]['hook_types'] == ['pre-commit']
+    assert patch.call_args[1]['hook_types'] is None
     assert patch.call_args[1]['skip_on_missing_config'] is True
 
 
