Simplify initialization with setup() API#19
Merged
Conversation
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)
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.
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.
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.
Remove '(required)' from modules parameter docstring since it's obviously required by the function signature.
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.
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
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.
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.
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.
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.
doodla
changed the title
doodla
deleted the
claude/investigate-octohook-setup-011CUq8mDMpXqTFDJh16oCvT
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit introduces a simplified setup API and better test isolation:
New Features:
setup(modules, model_overrides): One-time initialization with validation
reset(): Clear all hooks, modules, and overrides
OctohookConfigError: New exception for configuration errors
Testing Improvements: