Merge pull request #239 from callowayproject/236-fail-if-hooks-fail

Fail when hooks fail

.pre-commit-config.yaml

@@ -1,7 +1,7 @@
 repos:
   - repo: https://github.com/astral-sh/ruff-pre-commit
     # Ruff version.
-    rev: 'v0.6.7'
+    rev: 'v0.6.8'
     hooks:
       - id: ruff
         args: [--fix, --exit-non-zero-on-fix]

bumpversion/exceptions.py

@@ -59,3 +59,9 @@ class BadInputError(BumpVersionError):
     """User input was bad."""
 
     pass
+
+
+class HookError(BumpVersionError):
+    """A hook failed."""
+
+    pass

bumpversion/files.py

@@ -206,7 +206,7 @@ def make_file_change(
         if file_content_before == file_content_after and current_version.original:
             og_context = deepcopy(context)
             og_context["current_version"] = current_version.original
-            search_for_og, og_raw_search_pattern = self.file_change.get_search_pattern(og_context)
+            search_for_og, _ = self.file_change.get_search_pattern(og_context)
             file_content_after = search_for_og.sub(replace_with, file_content_before)
 
         log_changes(self.file_change.filename, file_content_before, file_content_after, dry_run)

bumpversion/hooks.py

@@ -7,6 +7,7 @@
 
 from bumpversion.config.models import Config
 from bumpversion.context import get_context
+from bumpversion.exceptions import HookError
 from bumpversion.ui import get_indented_logger
 from bumpversion.versioning.models import Version
 
@@ -21,7 +22,9 @@ def run_command(script: str, environment: Optional[dict] = None) -> subprocess.C
         raise TypeError(f"`script` must be a string, not {type(script)}")
     if environment and not isinstance(environment, dict):
         raise TypeError(f"`environment` must be a dict, not {type(environment)}")
-    return subprocess.run(script, env=environment, encoding="utf-8", shell=True, text=True, capture_output=True)
+    return subprocess.run(
+        script, env=environment, encoding="utf-8", shell=True, text=True, capture_output=True, check=False
+    )
 
 
 def base_env(config: Config) -> Dict[str, str]:
@@ -99,10 +102,15 @@ def run_hooks(hooks: List[str], env: Dict[str, str], dry_run: bool = False) -> N
         logger.debug(f"Running {script!r}")
         logger.indent()
         result = run_command(script, env)
-        logger.debug(result.stdout)
-        logger.debug(result.stderr)
-        logger.debug(f"Exited with {result.returncode}")
-        logger.indent()
+        if result.returncode != 0:
+            logger.warning(result.stdout)
+            logger.warning(result.stderr)
+            raise HookError(f"{script!r} exited with {result.returncode}. ")
+        else:
+            logger.debug(f"Exited with {result.returncode}")
+            logger.debug(result.stdout)
+            logger.debug(result.stderr)
+        logger.dedent()
     logger.dedent()
 
 

bumpversion/show.py

@@ -20,7 +20,7 @@ def output_default(value: dict) -> None:
         print_info(next(iter(value.values())))
     else:
         buffer = StringIO()
-        pprint(value, stream=buffer, sort_dicts=True)  # noqa: T203
+        pprint(value, stream=buffer, sort_dicts=True)
         print_info(buffer.getvalue())
 
 
@@ -77,6 +77,8 @@ def resolve_name(obj: Any, name: str, default: Any = None, err_on_missing: bool
 
     Raises:
         BadInputError: If we cannot resolve the name and `err_on_missing` is `True`
+        AttributeError: If a @property decorator raised it
+        TypeError: If a @property decorator raised it
 
     # noqa: DAR401
     """

bumpversion/utils.py

@@ -92,6 +92,7 @@ def set_nested_value(d: dict, value: Any, path: str) -> None:
 
     Raises:
         ValueError: If an element in the path is not a dictionary.
+        KeyError: If a key in the path does not exist.
     """
     keys = path.split(".")
     last_element = keys[-1]
@@ -103,7 +104,7 @@ def set_nested_value(d: dict, value: Any, path: str) -> None:
         elif key not in current_element:
             raise KeyError(f"Key '{key}' not found at '{'.'.join(keys[:keys.index(key)])}'")
         elif not isinstance(current_element[key], dict):
-            raise ValueError(f"Path '{'.'.join(keys[:i+1])}' does not lead to a dictionary.")
+            raise ValueError(f"Path '{'.'.join(keys[:i + 1])}' does not lead to a dictionary.")
         else:
             current_element = current_element[key]
 

bumpversion/versioning/version_config.py

@@ -76,7 +76,7 @@ def parse(self, version_string: Optional[str] = None, raise_error: bool = False)
             A Version object representing the string.
 
         Raises:
-            BumpversionException: If a version string is invalid and raise_error is True.
+            BumpVersionError: If a version string is invalid and raise_error is True.
         """
         parsed = parse_version(version_string, self.parse_regex.pattern)
 

pyproject.toml

@@ -183,20 +183,53 @@ exclude = [
 line-length = 119
 
 [tool.ruff.lint]
-select = ["E", "W", "F", "I", "N", "B", "BLE", "C", "D", "E", "F", "I", "N", "S", "T", "W", "RUF", "NPY", "PD", "PGH", "ANN", "C90", "PLC", "PLE", "PLW", "TCH"]
+preview = true
+select = [
+    "E", # pycodestyle errors
+    "W", # pycodestyle warnings
+    "F", # pyflakes
+    "I", # isort
+    "N", # PEP8 naming
+    "B", # flake8-bugbear
+    "BLE", # flake8-blind except
+    "D", # pydocstyle
+    # "DOC", # pydoclint
+    "S", # flakeu-bandit
+    "RUF", # Ruff-specific rules
+    "NPY", # NumPy-specific rules
+    "PD", # Pandas-vet
+    "PGH", # PyGrep hooks
+    "ANN", # flake8-annotations
+    "C90", # McCabe complexity
+    "PLC", # Pylint conventions
+    "PLE", # Pylint errors
+    "PLW", # Pylint warnings
+    "TCH", # Flake8 type-checking
+]
 ignore = [
-    "ANN002", "ANN003", "ANN101", "ANN102", "ANN204", "ANN401",
-    "S101", "S104", "S602",
-    "D105", "D106", "D107", "D200", "D212",
-    "PD011",
-    "PLW1510",
+    "ANN002", # missing-type-args
+    "ANN003", # missing-type-kwargs
+    "ANN101", # missing-type-self
+    "ANN102", # missing-type-cls
+    "ANN204", # missing-return-type-special-method
+    "ANN401", # any-type
+    "S101", # assert
+    "S104", # hardcoded-bind-all-interfaces
+    "S404", # suspicious-subprocess-import
+    "S602", # subprocess-popen-with-shell-equals-true
+    "D105", # undocumented-magic-method
+    "D106", # undocumented-public-nested-class
+    "D107", # undocumented-public-init
+    "D200", # fits-on-one-line
+    "D212", # multi-line-summary-first-line
+    "PD011", # pandas-use-of-dot-values
+    "PLC0415", # import-outside-toplevel
+    "PLW1641", # eq-without-hash
 ]
 
-fixable = ["E", "W", "F", "I", "N", "B", "BLE", "C", "D", "E", "F", "I", "N", "S", "T", "W", "RUF", "NPY", "PD", "PGH", "ANN", "C90", "PL", "PLC", "PLE", "PLW", "TCH"]
+fixable = ["ALL"]
 unfixable = []
 
-# Exclude a variety of commonly ignored directories.
-
 # Allow unused variables when underscore-prefixed.
 dummy-variable-rgx = "^(_+|(_+[a-zA-Z0-9_]*[a-zA-Z0-9]+?))$"
 
@@ -215,6 +248,12 @@ order-by-type = true
 [tool.ruff.lint.pydocstyle]
 convention = "google"
 
+[tool.ruff.lint.flake8-annotations]
+allow-star-arg-any = true
+mypy-init-return = true
+suppress-dummy-args = true
+suppress-none-returning = true
+
 [tool.bumpversion]
 current_version = "0.26.1"
 commit = true

tests/test_hooks/test_run_hooks.py

@@ -1,6 +1,9 @@
 """Tests for the run_hooks function."""
 
 from bumpversion import hooks
+import pytest
+
+from bumpversion.exceptions import HookError
 
 
 def test_calls_each_hook(mocker):
@@ -18,19 +21,40 @@ def test_calls_each_hook(mocker):
     # Assert
     expected_calls = [
         mocker.call("Running 'script1'"),
+        mocker.call("Exited with 0"),
         mocker.call("output"),
         mocker.call("error"),
-        mocker.call("Exited with 0"),
         mocker.call("Running 'script2'"),
+        mocker.call("Exited with 0"),
         mocker.call("output"),
         mocker.call("error"),
-        mocker.call("Exited with 0"),
     ]
     mock_logger.debug.assert_has_calls(expected_calls)
     mock_run_command.assert_any_call("script1", env)
     mock_run_command.assert_any_call("script2", env)
 
 
+def test_raises_exception_if_hook_fails(mocker):
+    """If a hook responds with an error, an exception should be raised."""
+    # Assemble
+    mock_logger = mocker.patch("bumpversion.hooks.logger")
+    mock_run_command = mocker.patch("bumpversion.hooks.run_command")
+    hooks_list = ["script1", "script2"]
+    env = {"var": "value"}
+    mock_run_command.return_value = mocker.MagicMock(stdout="output", stderr="error", returncode=1)
+
+    # Act
+    with pytest.raises(HookError):
+        hooks.run_hooks(hooks_list, env)
+
+    # Assert
+    expected_debug_calls = [mocker.call("Running 'script1'")]
+    expected_warning_calls = [mocker.call("output"), mocker.call("error")]
+    mock_logger.debug.assert_has_calls(expected_debug_calls)
+    mock_logger.warning.assert_has_calls(expected_warning_calls)
+    mock_run_command.assert_any_call("script1", env)
+
+
 def test_does_not_call_each_hook_when_dry_run(mocker):
     """It should not call each hook passed to it when dry_run is True."""
     # Assemble

tools/drawioexport.py

@@ -101,7 +101,7 @@ def export_file(
         "--scale",
         "2",
     ]
-    result = subprocess.run(cmd)  # noqa: S603
+    result = subprocess.run(cmd, check=False)  # noqa: S603
     return result.returncode
 
 
@@ -116,7 +116,7 @@ def export_file_if_needed(source: Path, page_index: int, dest_path: Path) -> Non
     if not use_cached_file(source, dest_path):
         export_file(source, page_index, dest_path, "svg")
     else:
-        print(f"Using cached file {dest_path}")  # noqa: T201
+        print(f"Using cached file {dest_path}")
 
 
 if __name__ == "__main__":

tools/update_frontmatter.py

@@ -57,7 +57,7 @@ def process_file(markdown_path: Path) -> None:
         for key, value in update.items():
             post[key] = value
         new_text = frontmatter.dumps(post)
-        print(f"Updating {markdown_path}")  # noqa: T201
+        print(f"Updating {markdown_path}")
         markdown_path.write_text(new_text, encoding="utf-8")