-
Notifications
You must be signed in to change notification settings - Fork 0
feat(implementation): add NormalImpl, WrapperImpl, and CompletionHook setup API #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: refactor/markers-attach-config
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| Hook implementations are now represented by dedicated types: | ||
| :class:`pluggy.NormalImpl` for normal implementations and | ||
| :class:`pluggy.WrapperImpl` for (old- and new-style) wrappers, both | ||
| subclasses of :class:`pluggy.HookImpl`. | ||
| ``HookimplConfiguration.create_hookimpl()`` selects the appropriate | ||
| subclass, and ``WrapperImpl.setup_and_get_completion_hook()`` exposes | ||
| wrapper setup/teardown as a ``CompletionHook`` callback. | ||
| ``HookImpl`` now stores its configuration as ``hookimpl_config``; the old | ||
| ``opts`` attribute remains as a deprecated alias property. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -193,7 +193,7 @@ def myhook(self): | |
| 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}>" | ||
|
Comment on lines
195
to
+196
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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}>"
)
|
||
| ) | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/WrapperImpland theCompletionHookprotocol 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
ValueErrorif 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.Call sites would then use
create_hook_impl(...)and never directly pickNormalImplvsWrapperImpl. You can also drop theValueErrorchecks in the subclasses because the factory is the single gatekeeper.2. Simplify
CompletionHookto a callable alias if you don’t need runtime typingIf you don’t rely on
isinstance(x, CompletionHook)/issubclasschecks, a protocol is heavier than necessary. A type alias keeps the signature clear without introducing an extra concept:The return type of
WrapperImpl.setup_and_get_completion_hookdoesn’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_hookThe nested
completion_hookfunction mixes argument extraction, wrapper generator preparation, and teardown orchestration. You can move the teardown logic into_executionso thatWrapperImplonly sets up the generator and delegates:This keeps the completion‑hook behaviour exactly as it is, but makes the teardown flow reusable, testable in isolation, and easier to read.