From c3ae058102babe46bf4140b6fe423fd8fc122cdf Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Nov 2025 01:15:52 +0000 Subject: [PATCH 01/11] Add setup() and reset() for improved initialization and testing This commit introduces a simplified setup API and better test isolation: New Features: - setup(modules, model_overrides): One-time initialization with validation - Required 'modules' parameter for explicit configuration - Optional 'model_overrides' with subclass validation - Always raises on import errors (no silent failures) - Warns on multiple calls and reconfigures - reset(): Clear all hooks, modules, and overrides - Primarily for testing isolation - Clears hook registry, imported modules, model overrides - Allows setup() to be called again - OctohookConfigError: New exception for configuration errors Testing Improvements: - Added autouse fixture in conftest.py that calls reset() before/after each test - Removes complex manual cleanup in test_decorator.py - Clears sys.modules for test modules to ensure fresh imports - Added comprehensive test suite in test_setup.py (12 tests) Documentation: - Updated README to show setup() as recommended approach - Updated CLAUDE.md with new architecture details - Marked load_hooks() as legacy but still supported Backwards Compatibility: - load_hooks() still works (uses strict=False internally) - model_overrides dict still accessible - All existing tests pass (195 tests) --- CLAUDE.md | 9 ++- README.md | 41 +++++++--- octohook/__init__.py | 102 ++++++++++++++++++++++++- tests/conftest.py | 29 ++++++++ tests/test_decorator.py | 31 -------- tests/test_setup.py | 161 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 327 insertions(+), 46 deletions(-) create mode 100644 tests/test_setup.py diff --git a/CLAUDE.md b/CLAUDE.md index 0bb74af..79bcdbd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,9 +24,12 @@ uv build ### Core Components **octohook/__init__.py** - Entry point providing: -- `load_hooks(modules)` - Recursively imports modules to register webhook handlers +- `setup(modules, model_overrides)` - One-time initialization (recommended). Validates model overrides and raises on import errors +- `load_hooks(modules)` - Recursively imports modules to register webhook handlers (legacy, logs errors but continues) +- `reset()` - Clears all hooks, imported modules, and model overrides (primarily for testing) - `model_overrides` - Global dict for extending/replacing model classes -- Exports: `hook`, `handle_webhook`, `parse`, `WebhookEvent`, `WebhookEventAction` +- `OctohookConfigError` - Exception raised for configuration errors +- Exports: `hook`, `handle_webhook`, `parse`, `setup`, `reset`, `WebhookEvent`, `WebhookEventAction`, `OctohookConfigError` **octohook/decorators.py** - Decorator system (`_WebhookDecorator` class): - `@hook(event, actions, repositories, debug)` - Registers functions as webhook handlers @@ -91,3 +94,5 @@ Octohook uses `Optional` types extensively and the `_optional()` helper to handl - Test fixtures in `tests/fixtures/complete/` contain real GitHub webhook payloads - Hook tests verify that the decorator system correctly routes events to handlers - Model tests verify parsing and URL interpolation +- `tests/conftest.py` provides an autouse fixture that calls `reset()` before/after each test for isolation +- The autouse fixture also clears test module imports from `sys.modules` to ensure decorators re-register on each test diff --git a/README.md b/README.md index 3399415..5927e2c 100644 --- a/README.md +++ b/README.md @@ -61,10 +61,10 @@ def webhook(): return Response(event.pull_request.head.user.name, status=200) ``` -### @hook +### @hook (Recommended) Alternatively, you can also let `octohook` do the heavy lifting of finding and executing the appropriate handlers for any given webhook. -The `@hook` decorator takes in four parameters, the `WebhookEvent`, a list of `WebhookEventAction`s, an optional list of repositories and a `debug` flag (defaults to `False`). +The `@hook` decorator takes in four parameters, the `WebhookEvent`, a list of `WebhookEventAction`s, an optional list of repositories and a `debug` flag (defaults to `False`). Any function this decorator is applied to is invoked whenever you receive an event with the specified `WebhookEvent` and a listed `WebhookEventAction`. @@ -82,7 +82,7 @@ def work(event: PullRequestEvent): `work()` is automatically called with the parsed `PullRequestEvent` anytime you receive a webhook event with `X-Github-Event: pull_request` and it has any of the `created` or `edited` actions. -If you don't specify a list of actions, then the function is invoked for _any_ action. For some events like `Push`, which do not have an `action`, take care not to specify any actions in the decorator. +If you don't specify a list of actions, then the function is invoked for _any_ action. For some events like `Push`, which do not have an `action`, take care not to specify any actions in the decorator. #### hooks/do_something.py ```python @@ -105,30 +105,53 @@ import octohook app = Flask(__name__) -octohook.load_hooks(["hooks"]) +# Initialize octohook (one-time setup) +octohook.setup(modules=["hooks"]) @app.route('/webhook', methods=['POST']) def webhook(): github_event = request.headers.get('X-GitHub-Event') - + octohook.handle_webhook(event_name=github_event, payload=request.json) return Response("OK", status=200) ``` -`handle_hooks` goes through all the handlers sequentially and blocks till everything is done. Any exceptions are logged to `logging.getLogger('octohook')`. You can configure the output stream of this logger to capture the logs. +`handle_webhook` goes through all the handlers sequentially and blocks till everything is done. Any exceptions are logged to `logging.getLogger('octohook')`. You can configure the output stream of this logger to capture the logs. + +**Note:** `octohook.load_hooks()` is still supported for backwards compatibility, but `octohook.setup()` is recommended as it provides better error handling and validation. ### Model Overrides -`octohook` provides a way to extend/modify the models being provided in the event object. `model_overrides` is a dictionary where you can map `octohook` models to your own. +`octohook` provides a way to extend/modify the models being provided in the event object. +#### Using setup() (Recommended) ```python import octohook from octohook.models import PullRequest class MyPullRequest(PullRequest): - + + def custom_work(self): + pass + +octohook.setup( + modules=["module_a"], + model_overrides={ + PullRequest: MyPullRequest + } +) +``` + +#### Manual override (Legacy) + +```python +import octohook +from octohook.models import PullRequest + +class MyPullRequest(PullRequest): + def custom_work(self): pass @@ -145,5 +168,5 @@ Check the [test](tests/test_model_override.py) for example usage. **Note** - The class is initialized with the relevant `payload: dict` data from the incoming event payload. -- It is recommended you subclass the original model class, but it is not required. +- It is recommended you subclass the original model class. When using `setup()`, this is validated automatically. - Type hints are no longer reliable for the overridden classes. diff --git a/octohook/__init__.py b/octohook/__init__.py index 6225de7..c5c8043 100644 --- a/octohook/__init__.py +++ b/octohook/__init__.py @@ -2,7 +2,7 @@ from importlib import import_module from pathlib import Path from pkgutil import walk_packages -from typing import List +from typing import List, Optional, Dict, Type from .decorators import hook, handle_webhook from .events import parse, WebhookEvent, WebhookEventAction @@ -15,6 +15,8 @@ "models", "model_overrides", "parse", + "reset", + "setup", "WebhookEvent", "WebhookEventAction", ] @@ -23,12 +25,20 @@ _imported_modules = [] model_overrides = {} +_setup_called = False -def _import_module(module: str) -> List[str]: + +class OctohookConfigError(Exception): + """Raised when octohook configuration is invalid.""" + pass + +def _import_module(module: str, strict: bool = True) -> List[str]: try: imported = import_module(module) except Exception as e: logger.error("Failed to import module %s", module, exc_info=e) + if strict: + raise return [] module_path = imported.__file__ @@ -42,7 +52,7 @@ def _import_module(module: str) -> List[str]: for _, module_name, is_package in walk_packages([str(package_dir)]): module_to_be_imported = f"{module}.{module_name}" if is_package: - imported_modules.extend(_import_module(module_to_be_imported)) + imported_modules.extend(_import_module(module_to_be_imported, strict=strict)) else: imported_modules.append(import_module(module_to_be_imported).__name__) return imported_modules @@ -57,4 +67,88 @@ def load_hooks(modules: List[str]): global _imported_modules for module in modules: - _imported_modules.extend(_import_module(module)) + _imported_modules.extend(_import_module(module, strict=False)) + + +def setup( + *, + modules: List[str], + model_overrides: Optional[Dict[Type, Type]] = None, +) -> None: + """ + Configure octohook (one-time initialization). + + Args: + modules: List of module paths to load hooks from (required). + model_overrides: Dict mapping base models to custom implementations. + Validates that overrides are subclasses. + + Raises: + OctohookConfigError: If configuration is invalid. + ImportError: If any module fails to import. + TypeError: If model_overrides contains invalid mappings. + + Example: + >>> octohook.setup(modules=["hooks.github", "hooks.slack"]) + >>> octohook.setup( + ... modules=["hooks"], + ... model_overrides={PullRequest: CustomPullRequest} + ... ) + """ + global _setup_called, _imported_modules + + # Warn if setup() called multiple times + if _setup_called: + logger.warning("octohook.setup() called multiple times - reconfiguring") + reset() + + _setup_called = True + + # Validate and set model overrides + if model_overrides: + for base_class, override_class in model_overrides.items(): + # Validate that override is a subclass of base + if not isinstance(override_class, type): + raise TypeError( + f"Model override for {base_class.__name__} must be a class, " + f"got {type(override_class).__name__}" + ) + if not issubclass(override_class, base_class): + raise TypeError( + f"Model override {override_class.__name__} must be a subclass of " + f"{base_class.__name__}" + ) + + # Access the module-level global via globals() + globals()["model_overrides"].update(model_overrides) + + # Load hook modules (strict=True, will raise on import errors) + for module in modules: + _imported_modules.extend(_import_module(module, strict=True)) + + +def reset() -> None: + """ + Reset octohook to initial state. + + Clears all registered hooks, imported modules, and model overrides. + Useful for testing isolation. + + Example: + >>> octohook.reset() + >>> octohook.setup(modules=["tests.hooks"]) + """ + global _imported_modules, model_overrides, _setup_called + + # Clear hook registry + from octohook.decorators import _decorator + _decorator.handlers.clear() + + # Clear imported modules tracking + _imported_modules.clear() + + # Clear model overrides + model_overrides.clear() + + # Reset setup flag + _setup_called = False diff --git a/tests/conftest.py b/tests/conftest.py index 16d7d14..2d66c57 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -9,9 +9,38 @@ from typing import List, Dict, Any import pytest +import octohook from octohook.decorators import _WebhookDecorator +@pytest.fixture(autouse=True) +def reset_octohook(): + """ + Reset octohook state before each test to ensure isolation. + + This fixture automatically runs before every test, clearing all hooks, + imported modules, and model overrides. It also clears test module imports + from sys.modules to ensure decorators re-register on each test. + """ + import sys + + # Clear octohook state + octohook.reset() + + # Save current sys.modules state + original_modules = set(sys.modules.keys()) + + yield + + # Cleanup after test + octohook.reset() + + # Remove test modules from sys.modules to allow fresh imports next test + for module_name in list(sys.modules.keys()): + if module_name not in original_modules and module_name.startswith("tests.hooks"): + sys.modules.pop(module_name, None) + + @pytest.fixture def isolated_decorator(): """ diff --git a/tests/test_decorator.py b/tests/test_decorator.py index af4e9b2..8317297 100644 --- a/tests/test_decorator.py +++ b/tests/test_decorator.py @@ -1,5 +1,3 @@ -import sys - import pytest import octohook @@ -24,35 +22,6 @@ EXPECTED_HANDLE_HOOKS_ONLY = 10 # Total hooks excluding debug_hooks module -@pytest.fixture(autouse=True) -def clean_imported_modules(monkeypatch): - """ - Clean up module imports after each test to prevent test pollution. - - This fixture: - 1. Saves current state of imported modules - 2. Resets octohook._imported_modules before test - 3. Cleans up sys.modules after test - - Using autouse=True applies this to all tests in this file automatically. - """ - # Save current imported modules - original_modules = set(sys.modules.keys()) - - # Reset octohook's module tracking - monkeypatch.setattr(octohook, "_imported_modules", []) - - yield # Run the test - - # Clean up any modules imported during test - for module_name in list(sys.modules.keys()): - if module_name not in original_modules and module_name.startswith("tests.hooks"): - sys.modules.pop(module_name, None) - - # Reset module tracking again - octohook._imported_modules = [] - - def test_load_hooks_calls_hook(mocker): """ Verify that load_hooks() correctly discovers and registers all hooks in test modules. diff --git a/tests/test_setup.py b/tests/test_setup.py new file mode 100644 index 0000000..a1e432e --- /dev/null +++ b/tests/test_setup.py @@ -0,0 +1,161 @@ +""" +Tests for octohook.setup() and octohook.reset() functions. + +These tests verify the new simplified initialization API. +""" +import pytest + +import octohook +from octohook import setup, reset, OctohookConfigError +from octohook.models import PullRequest +from octohook.events import WebhookEvent, WebhookEventAction + + +class CustomPullRequest(PullRequest): + """Test custom model class.""" + @property + def custom_property(self): + return "custom" + + +class InvalidClass: + """Not a subclass of any model.""" + pass + + +def test_setup_loads_modules(): + """Verify that setup() loads hooks from specified modules.""" + setup(modules=["tests.hooks.handle_hooks"]) + + # Verify hooks were loaded + from octohook.decorators import _decorator + assert len(_decorator.handlers) > 0 + + +def test_setup_with_model_overrides(): + """Verify that setup() applies model overrides correctly.""" + setup( + modules=["tests.hooks.handle_hooks"], + model_overrides={PullRequest: CustomPullRequest} + ) + + # Verify override was set + assert octohook.model_overrides[PullRequest] == CustomPullRequest + + +def test_setup_validates_model_override_is_subclass(): + """Verify that setup() validates model overrides are subclasses.""" + with pytest.raises(TypeError, match="must be a subclass"): + setup( + modules=["tests.hooks.handle_hooks"], + model_overrides={PullRequest: InvalidClass} + ) + + +def test_setup_validates_model_override_is_class(): + """Verify that setup() validates model overrides are classes.""" + with pytest.raises(TypeError, match="must be a class"): + setup( + modules=["tests.hooks.handle_hooks"], + model_overrides={PullRequest: "not a class"} + ) + + +def test_setup_raises_on_invalid_module(): + """Verify that setup() raises ImportError for invalid modules.""" + with pytest.raises(ModuleNotFoundError): + setup(modules=["nonexistent.module"]) + + +def test_setup_warns_on_multiple_calls(caplog): + """Verify that calling setup() multiple times logs a warning.""" + setup(modules=["tests.hooks.handle_hooks.label"]) + + # Second call should warn and reconfigure + setup(modules=["tests.hooks.debug_hooks"]) + + assert "called multiple times" in caplog.text + + +def test_setup_multiple_calls_replaces_hooks(): + """Verify that calling setup() multiple times replaces hooks.""" + setup(modules=["tests.hooks.handle_hooks.label"]) + + from octohook.decorators import _decorator + first_count = len(_decorator.handlers) + + # Second call should reset and load new hooks + setup(modules=["tests.hooks.debug_hooks"]) + + second_count = len(_decorator.handlers) + + # Should have different hooks loaded (debug_hooks only has LABEL event) + assert WebhookEvent.LABEL in _decorator.handlers + + +def test_reset_clears_hooks(): + """Verify that reset() clears all registered hooks.""" + setup(modules=["tests.hooks.handle_hooks"]) + + from octohook.decorators import _decorator + assert len(_decorator.handlers) > 0 + + reset() + + assert len(_decorator.handlers) == 0 + + +def test_reset_clears_model_overrides(): + """Verify that reset() clears model overrides.""" + setup( + modules=["tests.hooks.handle_hooks"], + model_overrides={PullRequest: CustomPullRequest} + ) + + assert len(octohook.model_overrides) > 0 + + reset() + + assert len(octohook.model_overrides) == 0 + + +def test_reset_clears_imported_modules(): + """Verify that reset() clears imported modules tracking.""" + setup(modules=["tests.hooks.handle_hooks"]) + + assert len(octohook._imported_modules) > 0 + + reset() + + assert len(octohook._imported_modules) == 0 + + +def test_reset_allows_setup_again(): + """Verify that reset() allows setup() to be called again without warning.""" + setup(modules=["tests.hooks.handle_hooks"]) + + reset() + + # Should not warn since we explicitly reset + setup(modules=["tests.hooks.debug_hooks"]) + + from octohook.decorators import _decorator + assert len(_decorator.handlers) > 0 + + +def test_setup_and_handle_webhook(fixture_loader): + """Integration test: setup() -> handle_webhook().""" + from tests.hooks import _tracker + + setup(modules=["tests.hooks.handle_hooks"]) + + # Load a test payload + payloads = fixture_loader.load("label") + created_payload = [p for p in payloads if p["action"] == "created"][0] + + _tracker.reset() + octohook.handle_webhook("label", created_payload) + + # Verify handlers were called + calls = _tracker.get_calls() + assert len(calls) > 0 From c45b6c067036bdfc0c82e7c502052b7d160a90e1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Nov 2025 03:47:38 +0000 Subject: [PATCH 02/11] Remove _setup_called flag, detect setup via handler presence Instead of maintaining a separate _setup_called boolean flag, we now check if handlers exist in the decorator to determine if setup() was already called. Changes: - Removed _setup_called global variable - Check _decorator.handlers to detect previous setup() calls - Cleaner - infers state from actual data instead of tracking it separately - Same behavior: warns on multiple calls and reconfigures All 195 tests pass. --- octohook/__init__.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/octohook/__init__.py b/octohook/__init__.py index c5c8043..332f025 100644 --- a/octohook/__init__.py +++ b/octohook/__init__.py @@ -25,7 +25,6 @@ _imported_modules = [] model_overrides = {} -_setup_called = False class OctohookConfigError(Exception): @@ -95,15 +94,14 @@ def setup( ... model_overrides={PullRequest: CustomPullRequest} ... ) """ - global _setup_called, _imported_modules + global _imported_modules - # Warn if setup() called multiple times - if _setup_called: + # Warn if setup() called multiple times (check if handlers already exist) + from octohook.decorators import _decorator + if _decorator.handlers: logger.warning("octohook.setup() called multiple times - reconfiguring") reset() - _setup_called = True - # Validate and set model overrides if model_overrides: for base_class, override_class in model_overrides.items(): @@ -138,7 +136,7 @@ def reset() -> None: >>> octohook.reset() >>> octohook.setup(modules=["tests.hooks"]) """ - global _imported_modules, model_overrides, _setup_called + global _imported_modules, model_overrides # Clear hook registry from octohook.decorators import _decorator @@ -149,6 +147,3 @@ def reset() -> None: # Clear model overrides model_overrides.clear() - - # Reset setup flag - _setup_called = False From 54a17af845cb98541eb3ca119462e964afc67b22 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Nov 2025 03:53:20 +0000 Subject: [PATCH 03/11] Remove strict parameter and clean up unnecessary comments Simplifications: - Removed 'strict' parameter from _import_module() - Both setup() and load_hooks() now always raise on import errors - Cleaner, more consistent behavior - no silent failures Code cleanup: - Removed unnecessary inline comments throughout the codebase - Code is self-documenting, comments were redundant - Cleaned up test files for better readability Files modified: - octohook/__init__.py: Removed strict parameter, cleaned comments - tests/conftest.py: Removed obvious comments in reset fixture - tests/test_setup.py: Removed redundant verification comments - CLAUDE.md: Updated docs to reflect both functions raise on errors All 195 tests pass. --- CLAUDE.md | 2 +- octohook/__init__.py | 31 +++++++------------------------ tests/conftest.py | 5 ----- tests/test_setup.py | 4 ---- 4 files changed, 8 insertions(+), 34 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 79bcdbd..c8c4131 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -25,7 +25,7 @@ uv build **octohook/__init__.py** - Entry point providing: - `setup(modules, model_overrides)` - One-time initialization (recommended). Validates model overrides and raises on import errors -- `load_hooks(modules)` - Recursively imports modules to register webhook handlers (legacy, logs errors but continues) +- `load_hooks(modules)` - Recursively imports modules to register webhook handlers (legacy). Also raises on import errors - `reset()` - Clears all hooks, imported modules, and model overrides (primarily for testing) - `model_overrides` - Global dict for extending/replacing model classes - `OctohookConfigError` - Exception raised for configuration errors diff --git a/octohook/__init__.py b/octohook/__init__.py index 332f025..b05bc84 100644 --- a/octohook/__init__.py +++ b/octohook/__init__.py @@ -31,15 +31,8 @@ class OctohookConfigError(Exception): """Raised when octohook configuration is invalid.""" pass -def _import_module(module: str, strict: bool = True) -> List[str]: - try: - imported = import_module(module) - except Exception as e: - logger.error("Failed to import module %s", module, exc_info=e) - if strict: - raise - return [] - +def _import_module(module: str) -> List[str]: + imported = import_module(module) module_path = imported.__file__ if module_path.endswith("__init__.py"): @@ -51,7 +44,7 @@ def _import_module(module: str, strict: bool = True) -> List[str]: for _, module_name, is_package in walk_packages([str(package_dir)]): module_to_be_imported = f"{module}.{module_name}" if is_package: - imported_modules.extend(_import_module(module_to_be_imported, strict=strict)) + imported_modules.extend(_import_module(module_to_be_imported)) else: imported_modules.append(import_module(module_to_be_imported).__name__) return imported_modules @@ -66,7 +59,7 @@ def load_hooks(modules: List[str]): global _imported_modules for module in modules: - _imported_modules.extend(_import_module(module, strict=False)) + _imported_modules.extend(_import_module(module)) def setup( @@ -95,17 +88,14 @@ def setup( ... ) """ global _imported_modules - - # Warn if setup() called multiple times (check if handlers already exist) from octohook.decorators import _decorator + if _decorator.handlers: logger.warning("octohook.setup() called multiple times - reconfiguring") reset() - # Validate and set model overrides if model_overrides: for base_class, override_class in model_overrides.items(): - # Validate that override is a subclass of base if not isinstance(override_class, type): raise TypeError( f"Model override for {base_class.__name__} must be a class, " @@ -117,12 +107,10 @@ def setup( f"{base_class.__name__}" ) - # Access the module-level global via globals() globals()["model_overrides"].update(model_overrides) - # Load hook modules (strict=True, will raise on import errors) for module in modules: - _imported_modules.extend(_import_module(module, strict=True)) + _imported_modules.extend(_import_module(module)) def reset() -> None: @@ -137,13 +125,8 @@ def reset() -> None: >>> octohook.setup(modules=["tests.hooks"]) """ global _imported_modules, model_overrides - - # Clear hook registry from octohook.decorators import _decorator - _decorator.handlers.clear() - # Clear imported modules tracking + _decorator.handlers.clear() _imported_modules.clear() - - # Clear model overrides model_overrides.clear() diff --git a/tests/conftest.py b/tests/conftest.py index 2d66c57..e8dbc1b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -24,18 +24,13 @@ def reset_octohook(): """ import sys - # Clear octohook state octohook.reset() - - # Save current sys.modules state original_modules = set(sys.modules.keys()) yield - # Cleanup after test octohook.reset() - # Remove test modules from sys.modules to allow fresh imports next test for module_name in list(sys.modules.keys()): if module_name not in original_modules and module_name.startswith("tests.hooks"): sys.modules.pop(module_name, None) diff --git a/tests/test_setup.py b/tests/test_setup.py index a1e432e..ede4ace 100644 --- a/tests/test_setup.py +++ b/tests/test_setup.py @@ -27,7 +27,6 @@ def test_setup_loads_modules(): """Verify that setup() loads hooks from specified modules.""" setup(modules=["tests.hooks.handle_hooks"]) - # Verify hooks were loaded from octohook.decorators import _decorator assert len(_decorator.handlers) > 0 @@ -39,7 +38,6 @@ def test_setup_with_model_overrides(): model_overrides={PullRequest: CustomPullRequest} ) - # Verify override was set assert octohook.model_overrides[PullRequest] == CustomPullRequest @@ -149,13 +147,11 @@ def test_setup_and_handle_webhook(fixture_loader): setup(modules=["tests.hooks.handle_hooks"]) - # Load a test payload payloads = fixture_loader.load("label") created_payload = [p for p in payloads if p["action"] == "created"][0] _tracker.reset() octohook.handle_webhook("label", created_payload) - # Verify handlers were called calls = _tracker.get_calls() assert len(calls) > 0 From f67cb59bf100fcc706aa6de23161c467327d3c4a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Nov 2025 03:59:03 +0000 Subject: [PATCH 04/11] BREAKING CHANGE: Remove load_hooks(), setup() is now the only way This is a breaking change that removes the legacy load_hooks() function entirely, making setup() the only supported initialization method. Changes: - Removed load_hooks() function from octohook/__init__.py - Removed load_hooks from __all__ exports - Added OctohookConfigError to __all__ exports (was missing) - Updated all tests to use setup() instead of load_hooks() - Updated README.md to remove legacy examples - Updated CLAUDE.md to remove load_hooks() reference Migration Guide: Before: octohook.load_hooks(["hooks"]) After: octohook.setup(modules=["hooks"]) Benefits: - One clear way to initialize octohook - Better error handling (always raises on import errors) - Model override validation built-in - Simpler API surface - Less confusion for new users All 195 tests pass. --- CLAUDE.md | 3 +-- README.md | 23 +-------------------- octohook/__init__.py | 14 +------------ tests/test_decorator.py | 46 ++++++++++++++++++++--------------------- 4 files changed, 26 insertions(+), 60 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c8c4131..a4aedd9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,8 +24,7 @@ uv build ### Core Components **octohook/__init__.py** - Entry point providing: -- `setup(modules, model_overrides)` - One-time initialization (recommended). Validates model overrides and raises on import errors -- `load_hooks(modules)` - Recursively imports modules to register webhook handlers (legacy). Also raises on import errors +- `setup(modules, model_overrides)` - One-time initialization. Validates model overrides and raises on import errors - `reset()` - Clears all hooks, imported modules, and model overrides (primarily for testing) - `model_overrides` - Global dict for extending/replacing model classes - `OctohookConfigError` - Exception raised for configuration errors diff --git a/README.md b/README.md index 5927e2c..7d9b41a 100644 --- a/README.md +++ b/README.md @@ -119,14 +119,10 @@ def webhook(): `handle_webhook` goes through all the handlers sequentially and blocks till everything is done. Any exceptions are logged to `logging.getLogger('octohook')`. You can configure the output stream of this logger to capture the logs. -**Note:** `octohook.load_hooks()` is still supported for backwards compatibility, but `octohook.setup()` is recommended as it provides better error handling and validation. - ### Model Overrides `octohook` provides a way to extend/modify the models being provided in the event object. -#### Using setup() (Recommended) - ```python import octohook from octohook.models import PullRequest @@ -144,23 +140,6 @@ octohook.setup( ) ``` -#### Manual override (Legacy) - -```python -import octohook -from octohook.models import PullRequest - -class MyPullRequest(PullRequest): - - def custom_work(self): - pass - -octohook.load_hooks(["module_a"]) -octohook.model_overrides = { - PullRequest: MyPullRequest -} -``` - Now, everytime `octohook` attempts to initialize a `PullRequest` object, it will initialize `MyPullRequest` instead. Check the [test](tests/test_model_override.py) for example usage. @@ -168,5 +147,5 @@ Check the [test](tests/test_model_override.py) for example usage. **Note** - The class is initialized with the relevant `payload: dict` data from the incoming event payload. -- It is recommended you subclass the original model class. When using `setup()`, this is validated automatically. +- You must subclass the original model class - `setup()` validates this automatically. - Type hints are no longer reliable for the overridden classes. diff --git a/octohook/__init__.py b/octohook/__init__.py index b05bc84..12d34ce 100644 --- a/octohook/__init__.py +++ b/octohook/__init__.py @@ -11,9 +11,9 @@ "events", "handle_webhook", "hook", - "load_hooks", "models", "model_overrides", + "OctohookConfigError", "parse", "reset", "setup", @@ -49,18 +49,6 @@ def _import_module(module: str) -> List[str]: imported_modules.append(import_module(module_to_be_imported).__name__) return imported_modules -def load_hooks(modules: List[str]): - """ - Iterates through the given modules recursively and imports all the modules to load hook information. - - @param modules List of modules to be imported. The modules cannot be relative. For example, you may use - "module_a.module_b" or "module_a", but not ".module_a" or "module_a/module_b" - """ - global _imported_modules - - for module in modules: - _imported_modules.extend(_import_module(module)) - def setup( *, diff --git a/tests/test_decorator.py b/tests/test_decorator.py index 8317297..fcde6dc 100644 --- a/tests/test_decorator.py +++ b/tests/test_decorator.py @@ -2,7 +2,7 @@ import octohook import octohook.decorators -from octohook import load_hooks +from octohook import setup from octohook.decorators import _WebhookDecorator from octohook.events import WebhookEvent, WebhookEventAction from tests.hooks import _tracker @@ -22,57 +22,57 @@ EXPECTED_HANDLE_HOOKS_ONLY = 10 # Total hooks excluding debug_hooks module -def test_load_hooks_calls_hook(mocker): +def test_setup_calls_hook(mocker): """ - Verify that load_hooks() correctly discovers and registers all hooks in test modules. + Verify that setup() correctly discovers and registers all hooks in test modules. The tests/hooks directory contains 22 total hooks across 6 files. This test - ensures all are discovered when load_hooks() is called with the parent module. + ensures all are discovered when setup() is called with the parent module. """ mock = mocker.patch("octohook.decorators.hook") - load_hooks(["tests.hooks"]) + setup(modules=["tests.hooks"]) assert mock.call_count == EXPECTED_TOTAL_HOOKS -def test_load_hooks_only_parses_specified_modules(mocker): +def test_setup_only_parses_specified_modules(mocker): """ - Verify that load_hooks() only loads hooks from specified modules. + Verify that setup() only loads hooks from specified modules. Tests that when a specific submodule is provided (e.g., tests.hooks.debug_hooks), only hooks from that module are loaded, not from sibling or parent modules. """ mock = mocker.patch("octohook.decorators.hook") - load_hooks(["tests.hooks.debug_hooks"]) + setup(modules=["tests.hooks.debug_hooks"]) assert mock.call_count == EXPECTED_DEBUG_HOOKS_ONLY -def test_load_hooks_parses_python_module(mocker): +def test_setup_parses_python_module(mocker): """ - Verify that load_hooks() can load a single Python module file. + Verify that setup() can load a single Python module file. - Tests that load_hooks() works with fully-qualified module paths pointing + Tests that setup() works with fully-qualified module paths pointing to individual Python files, not just packages. """ mock = mocker.patch("octohook.decorators.hook") - load_hooks(["tests.hooks.debug_hooks.label"]) + setup(modules=["tests.hooks.debug_hooks.label"]) assert mock.call_count == EXPECTED_DEBUG_HOOKS_ONLY -def test_load_hooks_parses_properly(mocker): +def test_setup_parses_properly(mocker): """ Verify that hooks are correctly organized in the three-level filtering structure. Tests the internal handler storage structure: event → action → repo → handlers. This ensures the filtering system (specific action + specific repo, wildcard action, - wildcard repo, etc.) is set up correctly after load_hooks() runs. + wildcard repo, etc.) is set up correctly after setup() runs. """ decorator = _WebhookDecorator() mocker.patch("octohook.decorators.hook", side_effect=decorator.webhook) - load_hooks(["tests.hooks"]) + setup(modules=["tests.hooks"]) handlers = decorator.handlers @@ -98,18 +98,18 @@ def test_load_hooks_parses_properly(mocker): assert len(review[WebhookEventAction.SUBMITTED]["doodla/octohook-playground2"]) == 1 -def test_calling_load_hooks_multiple_times_does_not_have_side_effects(mocker): +def test_calling_setup_multiple_times_does_not_have_side_effects(mocker): """ - Verify that calling load_hooks() multiple times doesn't register hooks multiple times. + Verify that calling setup() multiple times doesn't register hooks multiple times. - Tests that load_hooks() is idempotent - calling it repeatedly with the same + Tests that setup() is idempotent - calling it repeatedly with the same modules doesn't result in duplicate hook registrations or other side effects. """ mock = mocker.patch("octohook.decorators.hook") - load_hooks(["tests.hooks"]) - load_hooks(["tests.hooks"]) - load_hooks(["tests.hooks"]) + setup(modules=["tests.hooks"]) + setup(modules=["tests.hooks"]) + setup(modules=["tests.hooks"]) assert mock.call_count == EXPECTED_TOTAL_HOOKS @@ -128,7 +128,7 @@ def test_handle_hooks(mocker, fixture_loader): decorator = _WebhookDecorator() mocker.patch("octohook.decorators.hook", side_effect=decorator.webhook) - load_hooks(["tests.hooks.handle_hooks"]) + setup(modules=["tests.hooks.handle_hooks"]) # Expected hooks for each action (after removing inner/ directory) assertions = { @@ -208,7 +208,7 @@ def test_debug_hooks_are_handled(mocker, fixture_loader): decorator = _WebhookDecorator() mocker.patch("octohook.decorators.hook", side_effect=decorator.webhook) - load_hooks(["tests.hooks"]) + setup(modules=["tests.hooks"]) # LabelEvent has `debug=True`. Only debug hooks should be fired. label_events = fixture_loader.load("label") From d61f4e712941081b91c9e1b26fc2b77546a28b99 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Nov 2025 04:03:34 +0000 Subject: [PATCH 05/11] Clean up stale docstring language Remove '(required)' from modules parameter docstring since it's obviously required by the function signature. --- octohook/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/octohook/__init__.py b/octohook/__init__.py index 12d34ce..0d045f4 100644 --- a/octohook/__init__.py +++ b/octohook/__init__.py @@ -59,7 +59,7 @@ def setup( Configure octohook (one-time initialization). Args: - modules: List of module paths to load hooks from (required). + modules: List of module paths to load hooks from. model_overrides: Dict mapping base models to custom implementations. Validates that overrides are subclasses. From 80c0feb3425f6fa26f36fab0db6cfcaecbcb7ad4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Nov 2025 04:05:38 +0000 Subject: [PATCH 06/11] Remove stale '(Recommended)' from @hook section in README Since setup() is now the only way to initialize octohook, the @hook decorator doesn't need to be labeled as 'recommended' - it's just the way to use octohook with handlers. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 7d9b41a..94ef673 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,7 @@ def webhook(): return Response(event.pull_request.head.user.name, status=200) ``` -### @hook (Recommended) +### @hook Alternatively, you can also let `octohook` do the heavy lifting of finding and executing the appropriate handlers for any given webhook. The `@hook` decorator takes in four parameters, the `WebhookEvent`, a list of `WebhookEventAction`s, an optional list of repositories and a `debug` flag (defaults to `False`). From ba247053c08baefbfdd82e12702ab5a9da71044a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Nov 2025 04:12:19 +0000 Subject: [PATCH 07/11] Improve docstrings to be more professional and complete Changes to docstrings and documentation: - setup(): Removed "one-time initialization" language, added detailed explanation of behavior including recursive imports and reconfiguration handling. Improved example to show complete usage pattern. - reset(): Removed informal "Useful for testing" language, replaced with professional description of what the function does and its primary use case. - README.md: Changed comment from "Initialize octohook (one-time setup)" to "Load webhook handlers from the hooks module" - more descriptive and accurate. - CLAUDE.md: Updated function descriptions to be more complete and specific about what setup() and reset() do. - test_setup.py: Updated module docstring to be more specific about what is being tested. All docstrings now: - Avoid marketing/casual language - Clearly explain what the function does - Specify behavior and side effects - Include complete, realistic examples --- CLAUDE.md | 4 ++-- README.md | 2 +- octohook/__init__.py | 38 ++++++++++++++++++++++++++------------ tests/test_setup.py | 2 +- 4 files changed, 30 insertions(+), 16 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index a4aedd9..73be1ab 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,8 +24,8 @@ uv build ### Core Components **octohook/__init__.py** - Entry point providing: -- `setup(modules, model_overrides)` - One-time initialization. Validates model overrides and raises on import errors -- `reset()` - Clears all hooks, imported modules, and model overrides (primarily for testing) +- `setup(modules, model_overrides)` - Configures octohook by loading webhook handlers and registering model overrides. Validates that model overrides inherit from base classes. Raises on import errors. +- `reset()` - Clears all registered hooks, imported modules, and model overrides. Returns octohook to unconfigured state. - `model_overrides` - Global dict for extending/replacing model classes - `OctohookConfigError` - Exception raised for configuration errors - Exports: `hook`, `handle_webhook`, `parse`, `setup`, `reset`, `WebhookEvent`, `WebhookEventAction`, `OctohookConfigError` diff --git a/README.md b/README.md index 94ef673..aa66ce7 100644 --- a/README.md +++ b/README.md @@ -105,7 +105,7 @@ import octohook app = Flask(__name__) -# Initialize octohook (one-time setup) +# Load webhook handlers from the hooks module octohook.setup(modules=["hooks"]) @app.route('/webhook', methods=['POST']) diff --git a/octohook/__init__.py b/octohook/__init__.py index 0d045f4..3f0e451 100644 --- a/octohook/__init__.py +++ b/octohook/__init__.py @@ -56,22 +56,32 @@ def setup( model_overrides: Optional[Dict[Type, Type]] = None, ) -> None: """ - Configure octohook (one-time initialization). + Configure octohook by loading webhook handlers and registering model overrides. + + This function recursively imports the specified modules to discover and register + all decorated webhook handlers. If setup() has already been called, it will reset + the existing configuration and reconfigure with the new parameters. Args: - modules: List of module paths to load hooks from. - model_overrides: Dict mapping base models to custom implementations. - Validates that overrides are subclasses. + modules: List of fully-qualified module paths containing webhook handlers. + Modules are imported recursively. Cannot use relative imports. + model_overrides: Optional mapping of base model classes to custom subclasses. + All custom classes are validated to ensure they inherit from + the base class they override. Raises: - OctohookConfigError: If configuration is invalid. - ImportError: If any module fails to import. - TypeError: If model_overrides contains invalid mappings. + ImportError: If any specified module cannot be imported. + TypeError: If a model override is not a class or not a subclass of the base model. Example: - >>> octohook.setup(modules=["hooks.github", "hooks.slack"]) + >>> import octohook + >>> from octohook.models import PullRequest + >>> + >>> class CustomPullRequest(PullRequest): + ... pass + >>> >>> octohook.setup( - ... modules=["hooks"], + ... modules=["hooks.github", "hooks.slack"], ... model_overrides={PullRequest: CustomPullRequest} ... ) """ @@ -103,12 +113,16 @@ def setup( def reset() -> None: """ - Reset octohook to initial state. + Clear all octohook configuration and return to unconfigured state. + + This function removes all registered webhook handlers, clears the list of imported + modules, and removes all model overrides. After calling reset(), setup() must be + called again before handling webhooks. - Clears all registered hooks, imported modules, and model overrides. - Useful for testing isolation. + Primarily intended for use in test suites to ensure isolation between test cases. Example: + >>> import octohook >>> octohook.reset() >>> octohook.setup(modules=["tests.hooks"]) """ diff --git a/tests/test_setup.py b/tests/test_setup.py index ede4ace..2f83a71 100644 --- a/tests/test_setup.py +++ b/tests/test_setup.py @@ -1,7 +1,7 @@ """ Tests for octohook.setup() and octohook.reset() functions. -These tests verify the new simplified initialization API. +Verifies webhook handler loading, model override validation, and state management. """ import pytest From ec4f2844b1f9330b1faa817bad293649661c2d03 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Nov 2025 04:25:44 +0000 Subject: [PATCH 08/11] Simplify setup() and make model_overrides private Changes to setup(): - Always calls reset() first to ensure clean configuration - Removes conditional logic for detecting multiple calls - Model overrides now replace instead of update (more predictable) Changes to model_overrides: - Renamed model_overrides to _model_overrides (private) - Removed from __all__ exports - users should only use setup() - Updated all internal references and tests - No more globals() workaround needed for parameter name conflict Changes to reset(): - Updated docstring to reflect it's called by setup() - Removed "primarily for testing" language - it's core functionality Benefits: - Simpler setup() logic - no conditional reset - model_overrides is properly encapsulated - Clearer semantics - setup() always gives you a clean slate - Model overrides are replaced, not merged (less confusing) All 194 tests pass. --- CLAUDE.md | 13 ++++++++----- octohook/__init__.py | 31 ++++++++++++++----------------- octohook/models.py | 4 ++-- tests/test_edge_cases.py | 2 +- tests/test_model_override.py | 2 +- tests/test_setup.py | 16 +++------------- 6 files changed, 29 insertions(+), 39 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 73be1ab..8d06a39 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,9 +24,9 @@ uv build ### Core Components **octohook/__init__.py** - Entry point providing: -- `setup(modules, model_overrides)` - Configures octohook by loading webhook handlers and registering model overrides. Validates that model overrides inherit from base classes. Raises on import errors. +- `setup(modules, model_overrides)` - Configures octohook by loading webhook handlers and registering model overrides. Validates that model overrides inherit from base classes. Raises on import errors. Always calls reset() first to clear existing state. - `reset()` - Clears all registered hooks, imported modules, and model overrides. Returns octohook to unconfigured state. -- `model_overrides` - Global dict for extending/replacing model classes +- `_model_overrides` - Internal dict for extending/replacing model classes (private, set via setup()) - `OctohookConfigError` - Exception raised for configuration errors - Exports: `hook`, `handle_webhook`, `parse`, `setup`, `reset`, `WebhookEvent`, `WebhookEventAction`, `OctohookConfigError` @@ -65,7 +65,7 @@ Handler resolution order: ### Model Override System -Users can extend/replace models via `model_overrides`: +Users can extend/replace models via `setup()`: ```python from octohook.models import PullRequest @@ -74,10 +74,13 @@ class MyPullRequest(PullRequest): def custom_method(self): pass -octohook.model_overrides = {PullRequest: MyPullRequest} +octohook.setup( + modules=["hooks"], + model_overrides={PullRequest: MyPullRequest} +) ``` -When any model is instantiated, `BaseGithubModel.__new__` checks `model_overrides` and substitutes the custom class. This allows adding custom methods/properties to GitHub objects without modifying octohook source. +When any model is instantiated, `BaseGithubModel.__new__` checks `_model_overrides` and substitutes the custom class. This allows adding custom methods/properties to GitHub objects without modifying octohook source. ### Payload Inconsistencies diff --git a/octohook/__init__.py b/octohook/__init__.py index 3f0e451..d9bd201 100644 --- a/octohook/__init__.py +++ b/octohook/__init__.py @@ -12,7 +12,6 @@ "handle_webhook", "hook", "models", - "model_overrides", "OctohookConfigError", "parse", "reset", @@ -24,7 +23,7 @@ logger = logging.getLogger("octohook") _imported_modules = [] -model_overrides = {} +_model_overrides = {} class OctohookConfigError(Exception): @@ -58,9 +57,9 @@ def setup( """ Configure octohook by loading webhook handlers and registering model overrides. - This function recursively imports the specified modules to discover and register - all decorated webhook handlers. If setup() has already been called, it will reset - the existing configuration and reconfigure with the new parameters. + This function clears any existing configuration via reset(), then recursively + imports the specified modules to discover and register all decorated webhook + handlers. Args: modules: List of fully-qualified module paths containing webhook handlers. @@ -86,11 +85,8 @@ def setup( ... ) """ global _imported_modules - from octohook.decorators import _decorator - if _decorator.handlers: - logger.warning("octohook.setup() called multiple times - reconfiguring") - reset() + reset() if model_overrides: for base_class, override_class in model_overrides.items(): @@ -105,7 +101,7 @@ def setup( f"{base_class.__name__}" ) - globals()["model_overrides"].update(model_overrides) + globals()["_model_overrides"] = model_overrides.copy() for module in modules: _imported_modules.extend(_import_module(module)) @@ -115,20 +111,21 @@ def reset() -> None: """ Clear all octohook configuration and return to unconfigured state. - This function removes all registered webhook handlers, clears the list of imported - modules, and removes all model overrides. After calling reset(), setup() must be - called again before handling webhooks. + Removes all registered webhook handlers, clears the list of imported modules, + and removes all model overrides. After calling reset(), setup() must be called + again before handling webhooks. - Primarily intended for use in test suites to ensure isolation between test cases. + This function is automatically called by setup() to ensure a clean configuration. + It can also be called directly to clear octohook state. Example: >>> import octohook >>> octohook.reset() - >>> octohook.setup(modules=["tests.hooks"]) + >>> octohook.setup(modules=["hooks"]) """ - global _imported_modules, model_overrides + global _imported_modules, _model_overrides from octohook.decorators import _decorator _decorator.handlers.clear() _imported_modules.clear() - model_overrides.clear() + _model_overrides.clear() diff --git a/octohook/models.py b/octohook/models.py index e6c41c2..b1f206d 100644 --- a/octohook/models.py +++ b/octohook/models.py @@ -29,9 +29,9 @@ def _transform(url: str, local_variables: dict) -> str: class BaseGithubModel(ABC): def __new__(cls, *args, **kwargs): - from octohook import model_overrides + from octohook import _model_overrides - cls = model_overrides.get(cls) or cls + cls = _model_overrides.get(cls) or cls return object.__new__(cls) diff --git a/tests/test_edge_cases.py b/tests/test_edge_cases.py index a873fb5..45a7d1b 100644 --- a/tests/test_edge_cases.py +++ b/tests/test_edge_cases.py @@ -159,7 +159,7 @@ class MyUser(User): def custom_field(self): return "custom" - monkeypatch.setattr(octohook, "model_overrides", {User: MyUser}) + monkeypatch.setattr(octohook, "_model_overrides", {User: MyUser}) # Load fixture payload = fixture_loader.load("pull_request")[0] diff --git a/tests/test_model_override.py b/tests/test_model_override.py index b48fcac..f6d794c 100644 --- a/tests/test_model_override.py +++ b/tests/test_model_override.py @@ -20,7 +20,7 @@ def model_override(monkeypatch): the standard PullRequest class, allowing us to test the model override system. """ - monkeypatch.setattr(octohook, "model_overrides", { + monkeypatch.setattr(octohook, "_model_overrides", { PullRequest: MyPullRequest, }) return MyPullRequest diff --git a/tests/test_setup.py b/tests/test_setup.py index 2f83a71..d30d1e4 100644 --- a/tests/test_setup.py +++ b/tests/test_setup.py @@ -38,7 +38,7 @@ def test_setup_with_model_overrides(): model_overrides={PullRequest: CustomPullRequest} ) - assert octohook.model_overrides[PullRequest] == CustomPullRequest + assert octohook._model_overrides[PullRequest] == CustomPullRequest def test_setup_validates_model_override_is_subclass(): @@ -65,16 +65,6 @@ def test_setup_raises_on_invalid_module(): setup(modules=["nonexistent.module"]) -def test_setup_warns_on_multiple_calls(caplog): - """Verify that calling setup() multiple times logs a warning.""" - setup(modules=["tests.hooks.handle_hooks.label"]) - - # Second call should warn and reconfigure - setup(modules=["tests.hooks.debug_hooks"]) - - assert "called multiple times" in caplog.text - - def test_setup_multiple_calls_replaces_hooks(): """Verify that calling setup() multiple times replaces hooks.""" setup(modules=["tests.hooks.handle_hooks.label"]) @@ -110,11 +100,11 @@ def test_reset_clears_model_overrides(): model_overrides={PullRequest: CustomPullRequest} ) - assert len(octohook.model_overrides) > 0 + assert len(octohook._model_overrides) > 0 reset() - assert len(octohook.model_overrides) == 0 + assert len(octohook._model_overrides) == 0 def test_reset_clears_imported_modules(): From a6647f00dd6eb682bfb881283cc6470b5e557402 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Nov 2025 04:28:38 +0000 Subject: [PATCH 09/11] Remove unnecessary globals() workaround Since _model_overrides (global) and model_overrides (parameter) have different names, there's no shadowing conflict. We can use the standard global declaration instead of globals() dict access. Before: globals()["_model_overrides"] = model_overrides.copy() After: global _model_overrides _model_overrides = model_overrides.copy() Cleaner and more idiomatic Python. --- octohook/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/octohook/__init__.py b/octohook/__init__.py index d9bd201..4b7c2a4 100644 --- a/octohook/__init__.py +++ b/octohook/__init__.py @@ -84,7 +84,7 @@ def setup( ... model_overrides={PullRequest: CustomPullRequest} ... ) """ - global _imported_modules + global _imported_modules, _model_overrides reset() @@ -101,7 +101,7 @@ def setup( f"{base_class.__name__}" ) - globals()["_model_overrides"] = model_overrides.copy() + _model_overrides = model_overrides.copy() for module in modules: _imported_modules.extend(_import_module(module)) From 6a3353cd7e22a55cc98e9449ff1b20e7ad3d3143 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Nov 2025 05:12:49 +0000 Subject: [PATCH 10/11] Simplify autouse fixture - only handle sys.modules cleanup Since setup() now always calls reset() first, the fixture no longer needs to call reset() before/after each test. The only thing setup() doesn't handle is clearing test modules from sys.modules. Changes: - Renamed reset_octohook() to cleanup_test_modules() - Removed reset() calls (redundant with setup()) - Kept sys.modules cleanup (still critical for decorator re-registration) The sys.modules cleanup ensures @hook decorators can re-register on each test run, which is necessary because decorators execute at import time. All 194 tests pass. --- tests/conftest.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index e8dbc1b..9888066 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -14,23 +14,19 @@ @pytest.fixture(autouse=True) -def reset_octohook(): +def cleanup_test_modules(): """ - Reset octohook state before each test to ensure isolation. + Clean up test module imports after each test. - This fixture automatically runs before every test, clearing all hooks, - imported modules, and model overrides. It also clears test module imports - from sys.modules to ensure decorators re-register on each test. + Removes test hook modules from sys.modules to ensure decorators re-register + on each test. This is necessary because @hook decorators execute at import time. """ import sys - octohook.reset() original_modules = set(sys.modules.keys()) yield - octohook.reset() - for module_name in list(sys.modules.keys()): if module_name not in original_modules and module_name.startswith("tests.hooks"): sys.modules.pop(module_name, None) From e275e5cdd4c04ddc9260db7b4d736af8aa73f3b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Nov 2025 05:15:45 +0000 Subject: [PATCH 11/11] Validate and document why sys.modules cleanup is needed Tested removing the sys.modules cleanup - tests fail immediately. Test failure without cleanup: test_setup_only_parses_specified_modules failed with 0 hooks (expected 4) Root cause: 1. First test imports tests.hooks, decorators execute and register 2. setup() calls reset() clearing all handlers 3. Second test imports tests.hooks.debug_hooks 4. Python sees tests.hooks in sys.modules, doesn't re-import 5. Decorators don't re-execute, so 0 hooks registered The cleanup is essential - @hook decorators only execute at import time. Without clearing sys.modules, subsequent imports return cached modules and decorators never re-run. Enhanced docstring to explain this behavior more clearly. --- tests/conftest.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 9888066..a189ecc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -19,7 +19,8 @@ def cleanup_test_modules(): Clean up test module imports after each test. Removes test hook modules from sys.modules to ensure decorators re-register - on each test. This is necessary because @hook decorators execute at import time. + on each test. This is necessary because @hook decorators execute at import time, + and without cleanup, Python won't re-execute them when modules are imported again. """ import sys