feat(implementation): add NormalImpl, WrapperImpl, and CompletionHook setup API - #8
Open
RonnyPfannschmidt wants to merge 1 commit into
Open
Conversation
Reviewer's GuideRefactors hook implementation handling into a small hierarchy (HookImpl base with NormalImpl and WrapperImpl), centralizes hookimpl creation in HookimplConfiguration, and introduces a runtime-checkable CompletionHook API for wrapper setup/teardown, updating callers, exports, and tests accordingly. Sequence diagram for WrapperImpl.setup_and_get_completion_hook and completion hook teardownsequenceDiagram
participant Caller as HookCaller
participant W as WrapperImpl
participant Exec as _execution
participant Gen as wrapper_gen
Caller->>W: setup_and_get_completion_hook(hook_name, caller_kwargs)
W->>W: _get_call_args(caller_kwargs)
alt W.hookwrapper
W->>Exec: run_old_style_hookwrapper(W, hook_name, args)
Exec-->>W: wrapper_gen
else normal wrapper
W->>W: function(*args)
W-->>W: wrapper_gen
end
W->>Gen: next(wrapper_gen)
Gen-->>W: setup phase
W-->>Caller: completion_hook
Caller->>completion_hook: __call__(result, exception)
alt exception is not None
completion_hook->>Gen: throw(exception)
else no exception
completion_hook->>Gen: send(result)
end
completion_hook->>Gen: close()
alt wrapper completes
Gen-->>completion_hook: StopIteration(value)
completion_hook-->>Caller: (value, None)
else wrapper errors
Gen-->>completion_hook: BaseException(e)
completion_hook-->>Caller: (result, e)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was referenced Jul 24, 2026
refactor(caller): Protocol HookCaller, split callers, CompletionHook multicall
pytest-dev/pluggy#708
Open
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The deprecated
HookImpl.optsproperty currently only has a docstring note; consider emitting aDeprecationWarningon access so downstream code gets a runtime signal during migration. - Exposing
CompletionHook,NormalImpl, andWrapperImplvia__all__andpluggy.__init__effectively promotes them to public API; double-check that this is intentional and aligns with the desired stability and refactoring strategy.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The deprecated `HookImpl.opts` property currently only has a docstring note; consider emitting a `DeprecationWarning` on access so downstream code gets a runtime signal during migration.
- Exposing `CompletionHook`, `NormalImpl`, and `WrapperImpl` via `__all__` and `pluggy.__init__` effectively promotes them to public API; double-check that this is intentional and aligns with the desired stability and refactoring strategy.
## Individual Comments
### Comment 1
<location path="testing/test_implementation.py" line_range="180-193" />
<code_context>
+ assert isinstance(exception, RuntimeError)
+ assert "has second yield" in str(exception)
+
+ def test_old_style_hookwrapper_receives_result_object(self) -> None:
+ seen: list[object] = []
+
+ def old_style(arg: object) -> Generator[None, object, None]:
+ outcome = yield
+ seen.append(outcome)
+ outcome.force_result(f"forced: {arg}") # type: ignore[attr-defined]
+
+ impl = make_wrapper_impl(old_style, hookwrapper=True)
+ completion = impl.setup_and_get_completion_hook("myhook", {"arg": "a"})
+ result, exception = completion("orig", None)
+ assert result == "forced: a"
+ assert exception is None
+ assert seen and seen[0].__class__.__name__ == "Result"
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for exception handling with old-style hookwrappers
The existing tests for old-style hookwrappers only cover the happy-path `Result` and `force_result`. `WrapperImpl.setup_and_get_completion_hook` has more complex behaviour when `exception` is set and when the wrapper raises or swallows exceptions. For consistency with the new-style wrapper tests, please add at least one test where an old-style hookwrapper (a) inspects an exception and forces a result to swallow it, and (b) replaces it with a different exception. This will verify that `run_old_style_hookwrapper` interacts correctly with `CompletionHook` teardown in exceptional paths.
```suggestion
def test_old_style_hookwrapper_receives_result_object(self) -> None:
seen: list[object] = []
def old_style(arg: object) -> Generator[None, object, None]:
outcome = yield
seen.append(outcome)
outcome.force_result(f"forced: {arg}") # type: ignore[attr-defined]
impl = make_wrapper_impl(old_style, hookwrapper=True)
completion = impl.setup_and_get_completion_hook("myhook", {"arg": "a"})
result, exception = completion("orig", None)
assert result == "forced: a"
assert exception is None
assert seen and seen[0].__class__.__name__ == "Result"
def test_old_style_hookwrapper_swallow_exception_with_forced_result(self) -> None:
seen: list[object] = []
def old_style(arg: object) -> Generator[None, object, None]:
outcome = yield
seen.append(outcome)
# The old-style hookwrapper sees the original exception and decides to
# recover by forcing a successful result instead.
assert outcome.excinfo is not None # type: ignore[attr-defined]
assert isinstance(outcome.excinfo[1], ValueError) # type: ignore[attr-defined]
outcome.force_result(f"recovered: {arg}") # type: ignore[attr-defined]
impl = make_wrapper_impl(old_style, hookwrapper=True)
completion = impl.setup_and_get_completion_hook("myhook", {"arg": "a"})
result, exception = completion("orig", ValueError("boom"))
assert result == "recovered: a"
assert exception is None
assert seen and seen[0].__class__.__name__ == "Result"
def test_old_style_hookwrapper_replaces_exception(self) -> None:
seen: list[object] = []
def old_style(arg: object) -> Generator[None, object, None]:
outcome = yield
seen.append(outcome)
# The old-style hookwrapper inspects the original exception and then
# raises a different one to replace it.
assert outcome.excinfo is not None # type: ignore[attr-defined]
assert isinstance(outcome.excinfo[1], ValueError) # type: ignore[attr-defined]
raise RuntimeError(f"wrapper failed for: {arg}")
impl = make_wrapper_impl(old_style, hookwrapper=True)
completion = impl.setup_and_get_completion_hook("myhook", {"arg": "a"})
result, exception = completion("orig", ValueError("boom"))
assert result is None
assert isinstance(exception, RuntimeError)
assert "wrapper failed for: a" in str(exception)
assert seen and seen[0].__class__.__name__ == "Result"
```
</issue_to_address>
### Comment 2
<location path="testing/test_details.py" line_range="195-196" />
<code_context>
plugin = Plugin()
pname = pm.register(plugin)
assert repr(pm.hook.myhook.get_hookimpls()[0]) == (
- f"<HookImpl plugin_name={pname!r}, plugin={plugin!r}>"
+ f"<NormalImpl plugin_name={pname!r}, plugin={plugin!r}>"
)
</code_context>
<issue_to_address>
**suggestion (testing):** Add a complementary repr test for WrapperImpl to cover the new subclass-specific repr
NormalImpl’s repr is now asserted to include the concrete subclass name, and WrapperImpl shares the same repr logic but lacks a direct test. Please add a test that registers a wrapper implementation (e.g., via a small plugin using `@hookimpl(wrapper=True)`) and asserts its repr begins with `<WrapperImpl ...>` and includes the correct `plugin_name` and `plugin` values, so the new behavior is fully covered.
Suggested implementation:
```python
plugin = Plugin()
pname = pm.register(plugin)
assert repr(pm.hook.myhook.get_hookimpls()[0]) == (
f"<NormalImpl plugin_name={pname!r}, plugin={plugin!r}>"
)
class WrapperPlugin:
@hookimpl(wrapper=True)
def myhook(self, result):
return result
wrapper_plugin = WrapperPlugin()
wrapper_pname = pm.register(wrapper_plugin)
# WrapperImpl should be the second hook implementation for myhook
assert repr(pm.hook.myhook.get_hookimpls()[1]) == (
f"<WrapperImpl plugin_name={wrapper_pname!r}, plugin={wrapper_plugin!r}>"
)
```
1. Ensure `hookimpl` is already imported in `testing/test_details.py` (typically `from pluggy import HookimplMarker` or similar, aliased to `hookimpl`). If it is not, add the appropriate import using the existing conventions in the file.
2. Confirm that `pm` in this test has a `myhook` spec that accepts a `result` argument for wrapper implementations; if not, adjust the `WrapperPlugin.myhook` signature to match the defined hook spec.
3. If the `pm.register(plugin)` call changes the ordering of hook implementations (e.g., via `tryfirst`/`trylast` or other options in surrounding code), you may need to assert against the correct index for the `WrapperImpl` in `pm.hook.myhook.get_hookimpls()`.
</issue_to_address>
### Comment 3
<location path="src/pluggy/_implementation.py" line_range="30" />
<code_context>
-@final
+@runtime_checkable
+class CompletionHook(Protocol):
+ """Teardown callback returned by :meth:`WrapperImpl.setup_and_get_completion_hook`.
</code_context>
<issue_to_address>
**issue (complexity):** Consider reducing the exposed type-level surface by hiding the NormalImpl/WrapperImpl choice behind a factory, using a callable alias for CompletionHook, and extracting wrapper teardown into a separate helper.
The split into `NormalImpl` / `WrapperImpl` and the `CompletionHook` protocol adds quite a bit of surface area for the amount of new behaviour. You can keep all functionality while reducing the amount of “type-level” complexity by:
---
### 1. Hide the subclass distinction behind a factory
Right now callers must know which subclass to instantiate and are punished with `ValueError` if the config doesn’t match. Instead, centralise that logic and only expose a single creation entry point. This keeps the subclasses (and the wrapper‑specific methods) but removes the duplication and mental overhead at call sites.
```python
# helper near the class definitions
def create_hook_impl(
plugin: _Plugin,
plugin_name: str,
function: _HookImplFunction[object],
hook_impl_config: HookimplConfiguration,
) -> HookImpl:
if hook_impl_config.wrapper or hook_impl_config.hookwrapper:
return WrapperImpl(plugin, plugin_name, function, hook_impl_config)
return NormalImpl(plugin, plugin_name, function, hook_impl_config)
```
Call sites would then use `create_hook_impl(...)` and never directly pick `NormalImpl` vs `WrapperImpl`. You can also drop the `ValueError` checks in the subclasses because the factory is the single gatekeeper.
---
### 2. Simplify `CompletionHook` to a callable alias if you don’t need runtime typing
If you don’t rely on `isinstance(x, CompletionHook)` / `issubclass` checks, a protocol is heavier than necessary. A type alias keeps the signature clear without introducing an extra concept:
```python
CompletionHook: TypeAlias = Callable[
[object | list[object] | None, BaseException | None],
tuple[object | list[object] | None, BaseException | None],
]
```
The return type of `WrapperImpl.setup_and_get_completion_hook` doesn’t need to change beyond using this alias, and all current usage will keep working.
---
### 3. Extract the teardown orchestration from `WrapperImpl.setup_and_get_completion_hook`
The nested `completion_hook` function mixes argument extraction, wrapper generator preparation, and teardown orchestration. You can move the teardown logic into `_execution` so that `WrapperImpl` only sets up the generator and delegates:
```python
# in ._execution (or similar)
def run_wrapper_teardown(
wrapper_gen: Generator[None, object, object],
result: object | list[object] | None,
exception: BaseException | None,
) -> tuple[object | list[object] | None, BaseException | None]:
try:
if exception is not None:
try:
wrapper_gen.throw(exception)
except RuntimeError as re:
if isinstance(exception, StopIteration) and re.__cause__ is exception:
wrapper_gen.close()
return result, exception
raise
else:
wrapper_gen.send(result)
wrapper_gen.close()
_raise_wrapfail(wrapper_gen, "has second yield")
except StopIteration as si:
return si.value, None
except BaseException as e:
return result, e
```
```python
# in WrapperImpl
from ._execution import run_old_style_hookwrapper, run_wrapper_teardown
def setup_and_get_completion_hook(
self, hook_name: str, caller_kwargs: Mapping[str, object]
) -> CompletionHook:
args = self._get_call_args(caller_kwargs)
if self.hookwrapper:
wrapper_gen = run_old_style_hookwrapper(self, hook_name, args)
else:
wrapper_gen = cast(Generator[None, object, object], self.function(*args))
try:
next(wrapper_gen)
except StopIteration:
_raise_wrapfail(wrapper_gen, "did not yield")
def completion_hook(
result: object | list[object] | None,
exception: BaseException | None,
) -> tuple[object | list[object] | None, BaseException | None]:
return run_wrapper_teardown(wrapper_gen, result, exception)
return completion_hook
```
This keeps the completion‑hook behaviour exactly as it is, but makes the teardown flow reusable, testable in isolation, and easier to read.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+180
to
+193
| def test_old_style_hookwrapper_receives_result_object(self) -> None: | ||
| seen: list[object] = [] | ||
|
|
||
| def old_style(arg: object) -> Generator[None, object, None]: | ||
| outcome = yield | ||
| seen.append(outcome) | ||
| outcome.force_result(f"forced: {arg}") # type: ignore[attr-defined] | ||
|
|
||
| impl = make_wrapper_impl(old_style, hookwrapper=True) | ||
| completion = impl.setup_and_get_completion_hook("myhook", {"arg": "a"}) | ||
| result, exception = completion("orig", None) | ||
| assert result == "forced: a" | ||
| assert exception is None | ||
| assert seen and seen[0].__class__.__name__ == "Result" |
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for exception handling with old-style hookwrappers
The existing tests for old-style hookwrappers only cover the happy-path Result and force_result. WrapperImpl.setup_and_get_completion_hook has more complex behaviour when exception is set and when the wrapper raises or swallows exceptions. For consistency with the new-style wrapper tests, please add at least one test where an old-style hookwrapper (a) inspects an exception and forces a result to swallow it, and (b) replaces it with a different exception. This will verify that run_old_style_hookwrapper interacts correctly with CompletionHook teardown in exceptional paths.
Suggested change
| def test_old_style_hookwrapper_receives_result_object(self) -> None: | |
| seen: list[object] = [] | |
| def old_style(arg: object) -> Generator[None, object, None]: | |
| outcome = yield | |
| seen.append(outcome) | |
| outcome.force_result(f"forced: {arg}") # type: ignore[attr-defined] | |
| impl = make_wrapper_impl(old_style, hookwrapper=True) | |
| completion = impl.setup_and_get_completion_hook("myhook", {"arg": "a"}) | |
| result, exception = completion("orig", None) | |
| assert result == "forced: a" | |
| assert exception is None | |
| assert seen and seen[0].__class__.__name__ == "Result" | |
| def test_old_style_hookwrapper_receives_result_object(self) -> None: | |
| seen: list[object] = [] | |
| def old_style(arg: object) -> Generator[None, object, None]: | |
| outcome = yield | |
| seen.append(outcome) | |
| outcome.force_result(f"forced: {arg}") # type: ignore[attr-defined] | |
| impl = make_wrapper_impl(old_style, hookwrapper=True) | |
| completion = impl.setup_and_get_completion_hook("myhook", {"arg": "a"}) | |
| result, exception = completion("orig", None) | |
| assert result == "forced: a" | |
| assert exception is None | |
| assert seen and seen[0].__class__.__name__ == "Result" | |
| def test_old_style_hookwrapper_swallow_exception_with_forced_result(self) -> None: | |
| seen: list[object] = [] | |
| def old_style(arg: object) -> Generator[None, object, None]: | |
| outcome = yield | |
| seen.append(outcome) | |
| # The old-style hookwrapper sees the original exception and decides to | |
| # recover by forcing a successful result instead. | |
| assert outcome.excinfo is not None # type: ignore[attr-defined] | |
| assert isinstance(outcome.excinfo[1], ValueError) # type: ignore[attr-defined] | |
| outcome.force_result(f"recovered: {arg}") # type: ignore[attr-defined] | |
| impl = make_wrapper_impl(old_style, hookwrapper=True) | |
| completion = impl.setup_and_get_completion_hook("myhook", {"arg": "a"}) | |
| result, exception = completion("orig", ValueError("boom")) | |
| assert result == "recovered: a" | |
| assert exception is None | |
| assert seen and seen[0].__class__.__name__ == "Result" | |
| def test_old_style_hookwrapper_replaces_exception(self) -> None: | |
| seen: list[object] = [] | |
| def old_style(arg: object) -> Generator[None, object, None]: | |
| outcome = yield | |
| seen.append(outcome) | |
| # The old-style hookwrapper inspects the original exception and then | |
| # raises a different one to replace it. | |
| assert outcome.excinfo is not None # type: ignore[attr-defined] | |
| assert isinstance(outcome.excinfo[1], ValueError) # type: ignore[attr-defined] | |
| raise RuntimeError(f"wrapper failed for: {arg}") | |
| impl = make_wrapper_impl(old_style, hookwrapper=True) | |
| completion = impl.setup_and_get_completion_hook("myhook", {"arg": "a"}) | |
| result, exception = completion("orig", ValueError("boom")) | |
| assert result is None | |
| assert isinstance(exception, RuntimeError) | |
| assert "wrapper failed for: a" in str(exception) | |
| assert seen and seen[0].__class__.__name__ == "Result" |
Comment on lines
195
to
+196
| assert repr(pm.hook.myhook.get_hookimpls()[0]) == ( | ||
| f"<HookImpl plugin_name={pname!r}, plugin={plugin!r}>" | ||
| f"<NormalImpl plugin_name={pname!r}, plugin={plugin!r}>" |
There was a problem hiding this comment.
suggestion (testing): Add a complementary repr test for WrapperImpl to cover the new subclass-specific repr
NormalImpl’s repr is now asserted to include the concrete subclass name, and WrapperImpl shares the same repr logic but lacks a direct test. Please add a test that registers a wrapper implementation (e.g., via a small plugin using @hookimpl(wrapper=True)) and asserts its repr begins with <WrapperImpl ...> and includes the correct plugin_name and plugin values, so the new behavior is fully covered.
Suggested implementation:
plugin = Plugin()
pname = pm.register(plugin)
assert repr(pm.hook.myhook.get_hookimpls()[0]) == (
f"<NormalImpl plugin_name={pname!r}, plugin={plugin!r}>"
)
class WrapperPlugin:
@hookimpl(wrapper=True)
def myhook(self, result):
return result
wrapper_plugin = WrapperPlugin()
wrapper_pname = pm.register(wrapper_plugin)
# WrapperImpl should be the second hook implementation for myhook
assert repr(pm.hook.myhook.get_hookimpls()[1]) == (
f"<WrapperImpl plugin_name={wrapper_pname!r}, plugin={wrapper_plugin!r}>"
)
- Ensure
hookimplis already imported intesting/test_details.py(typicallyfrom pluggy import HookimplMarkeror similar, aliased tohookimpl). If it is not, add the appropriate import using the existing conventions in the file. - Confirm that
pmin this test has amyhookspec that accepts aresultargument for wrapper implementations; if not, adjust theWrapperPlugin.myhooksignature to match the defined hook spec. - If the
pm.register(plugin)call changes the ordering of hook implementations (e.g., viatryfirst/trylastor other options in surrounding code), you may need to assert against the correct index for theWrapperImplinpm.hook.myhook.get_hookimpls().
|
|
||
|
|
||
| @final | ||
| @runtime_checkable |
There was a problem hiding this comment.
issue (complexity): Consider reducing the exposed type-level surface by hiding the NormalImpl/WrapperImpl choice behind a factory, using a callable alias for CompletionHook, and extracting wrapper teardown into a separate helper.
The split into NormalImpl / WrapperImpl and the CompletionHook protocol adds quite a bit of surface area for the amount of new behaviour. You can keep all functionality while reducing the amount of “type-level” complexity by:
1. Hide the subclass distinction behind a factory
Right now callers must know which subclass to instantiate and are punished with ValueError if the config doesn’t match. Instead, centralise that logic and only expose a single creation entry point. This keeps the subclasses (and the wrapper‑specific methods) but removes the duplication and mental overhead at call sites.
# helper near the class definitions
def create_hook_impl(
plugin: _Plugin,
plugin_name: str,
function: _HookImplFunction[object],
hook_impl_config: HookimplConfiguration,
) -> HookImpl:
if hook_impl_config.wrapper or hook_impl_config.hookwrapper:
return WrapperImpl(plugin, plugin_name, function, hook_impl_config)
return NormalImpl(plugin, plugin_name, function, hook_impl_config)Call sites would then use create_hook_impl(...) and never directly pick NormalImpl vs WrapperImpl. You can also drop the ValueError checks in the subclasses because the factory is the single gatekeeper.
2. Simplify CompletionHook to a callable alias if you don’t need runtime typing
If you don’t rely on isinstance(x, CompletionHook) / issubclass checks, a protocol is heavier than necessary. A type alias keeps the signature clear without introducing an extra concept:
CompletionHook: TypeAlias = Callable[
[object | list[object] | None, BaseException | None],
tuple[object | list[object] | None, BaseException | None],
]The return type of WrapperImpl.setup_and_get_completion_hook doesn’t need to change beyond using this alias, and all current usage will keep working.
3. Extract the teardown orchestration from WrapperImpl.setup_and_get_completion_hook
The nested completion_hook function mixes argument extraction, wrapper generator preparation, and teardown orchestration. You can move the teardown logic into _execution so that WrapperImpl only sets up the generator and delegates:
# in ._execution (or similar)
def run_wrapper_teardown(
wrapper_gen: Generator[None, object, object],
result: object | list[object] | None,
exception: BaseException | None,
) -> tuple[object | list[object] | None, BaseException | None]:
try:
if exception is not None:
try:
wrapper_gen.throw(exception)
except RuntimeError as re:
if isinstance(exception, StopIteration) and re.__cause__ is exception:
wrapper_gen.close()
return result, exception
raise
else:
wrapper_gen.send(result)
wrapper_gen.close()
_raise_wrapfail(wrapper_gen, "has second yield")
except StopIteration as si:
return si.value, None
except BaseException as e:
return result, e# in WrapperImpl
from ._execution import run_old_style_hookwrapper, run_wrapper_teardown
def setup_and_get_completion_hook(
self, hook_name: str, caller_kwargs: Mapping[str, object]
) -> CompletionHook:
args = self._get_call_args(caller_kwargs)
if self.hookwrapper:
wrapper_gen = run_old_style_hookwrapper(self, hook_name, args)
else:
wrapper_gen = cast(Generator[None, object, object], self.function(*args))
try:
next(wrapper_gen)
except StopIteration:
_raise_wrapfail(wrapper_gen, "did not yield")
def completion_hook(
result: object | list[object] | None,
exception: BaseException | None,
) -> tuple[object | list[object] | None, BaseException | None]:
return run_wrapper_teardown(wrapper_gen, result, exception)
return completion_hookThis keeps the completion‑hook behaviour exactly as it is, but makes the teardown flow reusable, testable in isolation, and easier to read.
RonnyPfannschmidt
force-pushed
the
refactor/markers-attach-config
branch
from
July 24, 2026 16:38
6238439 to
e406f69
Compare
RonnyPfannschmidt
force-pushed
the
refactor/hookimpl-wrapper-types
branch
from
July 24, 2026 16:38
53041e2 to
c5cf018
Compare
RonnyPfannschmidt
force-pushed
the
refactor/markers-attach-config
branch
from
July 24, 2026 16:54
e406f69 to
5b654ff
Compare
RonnyPfannschmidt
force-pushed
the
refactor/hookimpl-wrapper-types
branch
from
July 24, 2026 16:54
c5cf018 to
6338f75
Compare
RonnyPfannschmidt
force-pushed
the
refactor/markers-attach-config
branch
from
August 12, 2026 10:50
5b654ff to
1ac246f
Compare
RonnyPfannschmidt
force-pushed
the
refactor/hookimpl-wrapper-types
branch
from
August 12, 2026 10:50
6338f75 to
6bb6aaf
Compare
RonnyPfannschmidt
force-pushed
the
refactor/markers-attach-config
branch
from
August 12, 2026 10:57
1ac246f to
4dc4e1f
Compare
RonnyPfannschmidt
force-pushed
the
refactor/hookimpl-wrapper-types
branch
from
August 12, 2026 10:57
6bb6aaf to
94a0f13
Compare
… setup API Complete design step 04: - HookImpl becomes a base class storing hookimpl_config (deprecated .opts alias kept) with arg binding moved to _get_call_args. - NormalImpl / WrapperImpl subclasses validate their configuration; HookimplConfiguration.create_hookimpl() returns the right subclass (fixing the try-claude footgun of bare HookImpl for normals). - WrapperImpl.setup_and_get_completion_hook() runs wrapper setup and returns a CompletionHook (runtime-checkable Protocol) that owns teardown, adapting old-style hookwrappers uniformly. - Registration and call_extra construct impls via create_hookimpl; multicall binds args via _get_call_args. Full dual-sequence multicall rewiring lands with design step 05. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
RonnyPfannschmidt
force-pushed
the
refactor/hookimpl-wrapper-types
branch
from
August 12, 2026 11:14
94a0f13 to
d059564
Compare
RonnyPfannschmidt
force-pushed
the
refactor/markers-attach-config
branch
from
August 12, 2026 11:14
4dc4e1f to
5840c41
Compare
This was referenced Aug 12, 2026
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
1 participant
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.
Review PR — step 4 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#707.
Merges happen upstream one step at a time, bottom-up. When step 4 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 04 of the internal-refactoring series (design/04-hookimpl-wrapper-types.md).
HookImpl becomes a base class holding hookimpl_config; NormalImpl / WrapperImpl validate their configuration; HookimplConfiguration.create_hookimpl() picks the subclass; WrapperImpl.setup_and_get_completion_hook() exposes wrapper setup/teardown as a runtime-checkable CompletionHook.
Stacked on the step-03 chain PR.
🤖 Generated with Claude Code
Summary by Sourcery
Introduce a typed HookImpl hierarchy with NormalImpl and WrapperImpl, add a CompletionHook-based wrapper setup/teardown API, and update configuration, registration, and public exports to construct and expose the appropriate implementation types.
New Features:
Enhancements:
Documentation:
Tests: