refactor(caller): Protocol HookCaller, split callers, CompletionHook multicall - #9
Conversation
Reviewer's GuideRefactors hook calling into a typed, protocol-based architecture with separate normal/historic/subset callers, splits normal vs wrapper implementations, reworks multicall orchestration via completion hooks, and updates PluginManager, decorators, tests, and public exports to the new structure. Sequence diagram for updated _multicall orchestrationsequenceDiagram
participant HC as NormalHookCaller
participant PM as PluginManager
participant MC as _multicall
participant WI as WrapperImpl
participant NI as NormalImpl
participant CH as CompletionHook
HC->>PM: _hookexec(hook_name, _normal_hookimpls, _wrapper_hookimpls, caller_kwargs, firstresult)
PM->>MC: _multicall(hook_name, normal_impls, wrapper_impls, caller_kwargs, firstresult)
loop setup wrappers
MC->>WI: setup_and_get_completion_hook(hook_name, caller_kwargs)
WI-->>MC: CompletionHook
MC->>CH: [store completion_hook]
end
loop run normal implementations
MC->>NI: _get_call_args(caller_kwargs)
NI-->>MC: args
MC->>NI: function(*args)
NI-->>MC: res
MC->>MC: [append to results / break if firstresult]
end
MC->>MC: [derive result]
loop run completion hooks LIFO
MC->>CH: __call__(result, exception)
CH-->>MC: (result, exception)
end
alt exception is not None
MC-->>PM: raise exception
else
MC-->>PM: return result
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/pluggy/_caller.py" line_range="121-125" />
<code_context>
+ """Call the hook historically."""
+ ...
+
+ def call_extra(
+ self, methods: Sequence[Callable[..., object]], kwargs: Mapping[str, object]
+ ) -> Any:
+ """Call the hook with additional methods."""
</code_context>
<issue_to_address>
**suggestion:** HistoricHookCaller.call_extra error message is misleading for unsupported functionality.
In `HistoricHookCaller.call_extra` the `AssertionError` message is reused from the direct historic hook call case: "Cannot directly call a historic hook - use call_historic instead." However, `call_extra` is not supported at all for historic hooks, which is a different situation. Consider using a more specific message (e.g., "Historic hooks do not support call_extra") to accurately describe the limitation and make debugging clearer, given that the method is present and documented.
```suggestion
def call_extra(
self, methods: Sequence[Callable[..., object]], kwargs: Mapping[str, object]
) -> Any:
"""Call the hook with additional methods."""
raise AssertionError("Historic hooks do not support call_extra")
```
</issue_to_address>
### Comment 2
<location path="testing/test_hookcaller.py" line_range="597-598" />
<code_context>
+ hc.set_specification(Hooks, HookspecConfiguration(historic=True))
+
+
+def test_normal_caller_rejects_call_historic(hc: NormalHookCaller) -> None:
+ with pytest.raises(AssertionError, match="not historic"):
+ hc.call_historic(kwargs=dict(arg=1))
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for argument verification warnings delegated to HookSpec.verify_all_args_are_provided
Argument verification was moved from `HookCaller._verify_all_args_are_provided` into `HookSpec.verify_all_args_are_provided`, with both `NormalHookCaller` and `HistoricHookCaller` delegating to it, but there are no tests ensuring the warning is still emitted when required arguments are missing.
Please add tests that:
- Call a normal hook while omitting a declared argument and use `pytest.warns` to assert the warning type and message.
- Do the same for a historic hook via `call_historic`.
This will protect the user-facing warning behavior across future refactors of `verify_all_args_are_provided`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def call_extra( | ||
| self, methods: Sequence[Callable[..., object]], kwargs: Mapping[str, object] | ||
| ) -> Any: | ||
| """Call the hook with additional methods.""" | ||
| ... |
There was a problem hiding this comment.
suggestion: HistoricHookCaller.call_extra error message is misleading for unsupported functionality.
In HistoricHookCaller.call_extra the AssertionError message is reused from the direct historic hook call case: "Cannot directly call a historic hook - use call_historic instead." However, call_extra is not supported at all for historic hooks, which is a different situation. Consider using a more specific message (e.g., "Historic hooks do not support call_extra") to accurately describe the limitation and make debugging clearer, given that the method is present and documented.
| def call_extra( | |
| self, methods: Sequence[Callable[..., object]], kwargs: Mapping[str, object] | |
| ) -> Any: | |
| """Call the hook with additional methods.""" | |
| ... | |
| def call_extra( | |
| self, methods: Sequence[Callable[..., object]], kwargs: Mapping[str, object] | |
| ) -> Any: | |
| """Call the hook with additional methods.""" | |
| raise AssertionError("Historic hooks do not support call_extra") |
| def test_normal_caller_rejects_call_historic(hc: NormalHookCaller) -> None: | ||
| with pytest.raises(AssertionError, match="not historic"): |
There was a problem hiding this comment.
suggestion (testing): Add tests for argument verification warnings delegated to HookSpec.verify_all_args_are_provided
Argument verification was moved from HookCaller._verify_all_args_are_provided into HookSpec.verify_all_args_are_provided, with both NormalHookCaller and HistoricHookCaller delegating to it, but there are no tests ensuring the warning is still emitted when required arguments are missing.
Please add tests that:
- Call a normal hook while omitting a declared argument and use
pytest.warnsto assert the warning type and message. - Do the same for a historic hook via
call_historic.
This will protect the user-facing warning behavior across future refactors of verify_all_args_are_provided.
53041e2 to
c5cf018
Compare
a2063ae to
ea1ab94
Compare
c5cf018 to
6338f75
Compare
ea1ab94 to
0bd7b6b
Compare
6338f75 to
6bb6aaf
Compare
0bd7b6b to
9e5b222
Compare
6bb6aaf to
94a0f13
Compare
9e5b222 to
15e5147
Compare
…multicall Complete design step 05: - HookCaller is now a @runtime_checkable Protocol; concrete callers are NormalHookCaller (split list[NormalImpl] / list[WrapperImpl] storage), HistoricHookCaller (memorize/replay, rejects wrappers) and SubsetHookCaller (read-only filtered proxy). _HookCaller and _SubsetHookCaller remain as compat aliases. - _multicall takes dual sequences and orchestrates phases only: wrapper setup collects CompletionHooks, normals run, completion hooks run LIFO and may replace (result, exception) - no wrapper flag branching. - add_hookspecs hands a NormalHookCaller over to a HistoricHookCaller when a historic spec arrives after impl registration. - PluginManager._hookexec and tracing use the dual-sequence signature; monitoring callbacks keep receiving one combined impl list. - HookSpec.verify_all_args_are_provided replaces the caller-side helper; set_specification accepts a config object or legacy mapping (shim). - New tests: protocol isinstance for all concretes, historic handover, historic direct-call/call_extra rejection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
94a0f13 to
d059564
Compare
15e5147 to
dce483a
Compare
Review PR — step 5 of 7.
This PR targets the previous step's branch, so its diff is only this step's change. Review happens here. The corresponding upstream PR, which is the one that actually merges, is pytest-dev#708.
Merges happen upstream one step at a time, bottom-up. When step 5 lands upstream, this PR is closed and the rest of the stack is rebased onto the new
main.refactor/split-hook-modulesrefactor/configuration-objectsrefactor/markers-attach-configrefactor/hookimpl-wrapper-typesrefactor/hookcaller-and-executionrefactor/project-specrefactor/async-submitterChain step 05 of the internal-refactoring series (design/05-hookcaller-and-execution.md).
HookCaller becomes a @runtime_checkable Protocol over NormalHookCaller (split normal/wrapper lists), HistoricHookCaller and SubsetHookCaller; _multicall is dual-sequence phase orchestration with LIFO CompletionHook teardown and no wrapper flag branching; historic specs arriving after impl registration hand over to a HistoricHookCaller; tracing keeps the combined-list callback shape.
Stacked on the step-04 chain PR.
🤖 Generated with Claude Code
Summary by Sourcery
Refactor hook calling infrastructure to split normal and wrapper implementations, introduce protocol-based HookCaller variants, and update multicall execution and plugin manager to support historic hooks and subset callers while preserving tracing and public APIs.
New Features:
Enhancements:
Documentation:
Tests: