Skip to content

refactor(caller): Protocol HookCaller, split callers, CompletionHook multicall - #9

Open
RonnyPfannschmidt wants to merge 1 commit into
refactor/hookimpl-wrapper-typesfrom
refactor/hookcaller-and-execution
Open

refactor(caller): Protocol HookCaller, split callers, CompletionHook multicall#9
RonnyPfannschmidt wants to merge 1 commit into
refactor/hookimpl-wrapper-typesfrom
refactor/hookcaller-and-execution

Conversation

@RonnyPfannschmidt

@RonnyPfannschmidt RonnyPfannschmidt commented Jul 24, 2026

Copy link
Copy Markdown
Owner

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.

Step Branch Review (downstream) Merge (upstream)
1 refactor/split-hook-modules #6 pytest-dev#703
2 refactor/configuration-objects #5 pytest-dev#704
3 refactor/markers-attach-config #7 pytest-dev#706
4 refactor/hookimpl-wrapper-types #8 pytest-dev#707
5 refactor/hookcaller-and-execution #9 pytest-dev#708
6 refactor/project-spec #10 pytest-dev#709
7 refactor/async-submitter #11 pytest-dev#710

Chain 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:

  • Expose NormalHookCaller, HistoricHookCaller, and SubsetHookCaller as concrete hook caller types implementing a runtime-checkable HookCaller protocol.
  • Support historic hook specifications via a dedicated HistoricHookCaller that records call history and replays it for late-registered plugins.
  • Allow subset_hook_caller to proxy both normal and historic hook callers with filtered implementations.

Enhancements:

  • Split hook implementations into separate normal and wrapper lists and adjust multicall orchestration to use completion hooks with LIFO teardown.
  • Move hookspec argument verification into HookSpec and reuse it across caller implementations.
  • Update tracing, benchmarks, and tests to work with the new hook caller and multicall shapes while keeping callback signatures compatible.

Documentation:

  • Add a changelog entry describing the HookCaller refactor and historic hook handling changes.

Tests:

  • Extend and adjust hookcaller and multicall tests to cover the new NormalHookCaller, HistoricHookCaller, SubsetHookCaller, and historic hand-over behavior.

@sourcery-ai

sourcery-ai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Reviewer's Guide

Refactors 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 orchestration

sequenceDiagram
    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
Loading

File-Level Changes

Change Details Files
Introduce HookCaller as a runtime-checkable Protocol and split concrete implementations into NormalHookCaller, HistoricHookCaller, and SubsetHookCaller with clearer responsibilities and data layout.
  • Define HookCaller Protocol with the public hook-calling interface (name/spec accessors, has_spec/is_historic, get_hookimpls, set_specification, call, call_historic, call_extra).
  • Replace the old monolithic HookCaller class with NormalHookCaller that manages separate normal and wrapper impl lists, delegates arg verification to HookSpec, and provides call_extra and history-noop behavior.
  • Add HistoricHookCaller dedicated to historic hooks with its own impl list, call history storage/replay, restricted spec modification, and explicit rejection of direct call and call_extra.
  • Add SubsetHookCaller as a read-only proxy over NormalHookCaller/HistoricHookCaller that filters implementations by plugin, preserves history behavior, and adapts calls/call_extra to the split impl lists.
  • Add TYPE_CHECKING block to assert concrete callers satisfy the HookCaller protocol and keep historical aliases (_HookCaller, _SubsetHookCaller) for backward compatibility.
src/pluggy/_caller.py
src/pluggy/_hooks.py
Refactor multicall execution to operate on separate normal and wrapper implementation sequences using CompletionHook-based teardown orchestration.
  • Change _HookExec signature and _multicall to accept separate normal_impls and wrapper_impls sequences instead of a single HookImpl list.
  • Adjust run_old_style_hookwrapper and teardown warning helpers to work specifically with WrapperImpl and update warning stacklevel for more accurate reporting.
  • Implement wrapper phase as setup via WrapperImpl.setup_and_get_completion_hook collecting CompletionHook objects, then run normal implementations, then apply completion hooks in LIFO order to transform (result, exception).
  • Update all call sites (PluginManager._hookexec, tracing wrapper, tests, benchmark helpers, HistoricHookCaller, SubsetHookCaller) to build and pass split normal/wrapper impl lists into _multicall.
  • Ensure historic hooks call _multicall with only normal impls and an empty wrapper list, preserving historic semantics while fitting the new signature.
src/pluggy/_execution.py
src/pluggy/_manager.py
testing/test_multicall.py
testing/benchmark.py
Tighten HookspecConfiguration and HookSpec usage, centralize argument verification, and support legacy spec configuration mappings.
  • Add _coerce_spec_config helper to accept either HookspecConfiguration or a legacy Mapping and convert via hookspec_config_from_mapping.
  • Modify NormalHookCaller.set_specification to coerce config and explicitly reject historic=True specs, directing such use to HistoricHookCaller.
  • Add HookSpec.verify_all_args_are_provided method that encapsulates the warning logic for missing hook call arguments, with an adjusted stacklevel.
  • Change NormalHookCaller and HistoricHookCaller to delegate argument verification to HookSpec.verify_all_args_are_provided instead of local warning code.
  • Wire HookspecConfiguration(opts) backward-compat property to continue working while encouraging use of config and new verification method.
src/pluggy/_caller.py
src/pluggy/_config.py
src/pluggy/_decorators.py
Update PluginManager to construct and manage the new caller types and NormalImpl/WrapperImpl instances, including historic-spec handover behavior.
  • Adjust imports to bring in NormalHookCaller, HistoricHookCaller, SubsetHookCaller, NormalImpl, WrapperImpl and to use HookCaller only as the Protocol type.
  • Update _hookexec and tracing wrapper to work with split normal/wrapper impl lists, while keeping before/after monitoring callbacks on a combined HookImpl list for backwards compatibility.
  • Change register to create NormalImpl/WrapperImpl via HookimplConfiguration.create_hookimpl and to attach either new NormalHookCaller or HistoricHookCaller depending on spec, asserting types when unregistering.
  • Enhance add_hookspecs to construct HistoricHookCaller for historic specs, and add logic that when a historic spec appears after implementations are registered, it creates a HistoricHookCaller, transfers existing impls, verifies them, and swaps the caller on HookRelay.
  • Adjust check_pending and subset_hook_caller to work with NormalHookCaller/HistoricHookCaller and SubsetHookCaller, including proper repr expectations in tests.
src/pluggy/_manager.py
testing/test_pluginmanager.py
Expose the new caller types and impl classes at the public API level and update tests to cover protocol behavior, historic transitions, and ordering with the refactored model.
  • Export NormalHookCaller, HistoricHookCaller, SubsetHookCaller, NormalImpl, WrapperImpl from pluggy.init and from _hooks.all so users can import them directly.
  • Update testing for hook calling (test_hookcaller.py) to use NormalHookCaller/HistoricHookCaller explicitly, construct hookimpls via HookimplConfiguration.create_hookimpl, and validate ordering for normal vs wrapper impl lists.
  • Add tests to verify HookCaller is a runtime-checkable protocol satisfied by NormalHookCaller, HistoricHookCaller, and SubsetHookCaller, and that historic specs arriving after registration hand over implementations correctly.
  • Add tests ensuring HistoricHookCaller rejects direct call and call_extra, enforcing use of call_historic, and that NormalHookCaller rejects call_historic.
  • Adjust multicall tests and benchmarking helpers to operate on NormalImpl/WrapperImpl and the new _multicall signature.
  • Add a changelog entry documenting this feature under 707.feature.rst and update API docs reference file stub.
src/pluggy/__init__.py
testing/test_hookcaller.py
testing/test_multicall.py
testing/benchmark.py
docs/api_reference.rst
changelog/707.feature.rst

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread src/pluggy/_caller.py
Comment on lines +121 to +125
def call_extra(
self, methods: Sequence[Callable[..., object]], kwargs: Mapping[str, object]
) -> Any:
"""Call the hook with additional methods."""
...

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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")

Comment on lines +597 to +598
def test_normal_caller_rejects_call_historic(hc: NormalHookCaller) -> None:
with pytest.raises(AssertionError, match="not historic"):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the refactor/hookimpl-wrapper-types branch from 53041e2 to c5cf018 Compare July 24, 2026 16:38
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the refactor/hookcaller-and-execution branch from a2063ae to ea1ab94 Compare July 24, 2026 16:38
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the refactor/hookimpl-wrapper-types branch from c5cf018 to 6338f75 Compare July 24, 2026 16:54
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the refactor/hookcaller-and-execution branch from ea1ab94 to 0bd7b6b Compare July 24, 2026 16:54
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the refactor/hookimpl-wrapper-types branch from 6338f75 to 6bb6aaf Compare August 12, 2026 10:50
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the refactor/hookcaller-and-execution branch from 0bd7b6b to 9e5b222 Compare August 12, 2026 10:50
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the refactor/hookimpl-wrapper-types branch from 6bb6aaf to 94a0f13 Compare August 12, 2026 10:57
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the refactor/hookcaller-and-execution branch from 9e5b222 to 15e5147 Compare August 12, 2026 10:57
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant