feat(config): replace TypedDict options with Hook*Configuration - #5
Open
RonnyPfannschmidt wants to merge 1 commit into
Open
feat(config): replace TypedDict options with Hook*Configuration#5RonnyPfannschmidt wants to merge 1 commit into
RonnyPfannschmidt wants to merge 1 commit into
Conversation
4 tasks
Reviewer's GuideIntroduce HookspecConfiguration and HookimplConfiguration as the primary hook config objects, migrate internal registration/calling logic and markers to use them, and keep legacy dict/TypedDict options available via a pytest compatibility module and deprecated shims. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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
__repr__implementations forHookspecConfigurationandHookimplConfigurationskip falsy values, which makes it hard to see flags explicitly set toFalseorNone; consider including all slots so configuration state is fully inspectable. - The broad
except Exceptionaroundgetattrin_read_hookimpl_configurationand_read_hookspec_configurationmay hide real bugs in plugin/spec code; tightening this to specific exception types or at least logging unexpected errors would make configuration discovery easier to debug.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `__repr__` implementations for `HookspecConfiguration` and `HookimplConfiguration` skip falsy values, which makes it hard to see flags explicitly set to `False` or `None`; consider including all slots so configuration state is fully inspectable.
- The broad `except Exception` around `getattr` in `_read_hookimpl_configuration` and `_read_hookspec_configuration` may hide real bugs in plugin/spec code; tightening this to specific exception types or at least logging unexpected errors would make configuration discovery easier to debug.
## Individual Comments
### Comment 1
<location path="src/pluggy/_manager.py" line_range="219" />
<code_context>
+ config = self._read_hookimpl_configuration(plugin, name)
+ if config is None:
+ return None
+ return cast(HookimplOpts, hookimpl_config_to_mapping(config))
def unregister(
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `cast` here will raise at runtime unless `cast` is imported in this module.
`cast` must be imported (e.g., `from typing import cast`) for this call to work; otherwise `parse_hookimpl_opts` will raise a `NameError` at runtime in this compatibility path.
</issue_to_address>
### Comment 2
<location path="src/pluggy/_manager.py" line_range="350" />
<code_context>
+ config = self._read_hookspec_configuration(module_or_class, name)
+ if config is None:
+ return None
+ return cast(HookspecOpts, hookspec_config_to_mapping(config))
def get_plugins(self) -> set[Any]:
</code_context>
<issue_to_address>
**issue (bug_risk):** Same `cast` runtime issue in `parse_hookspec_opts` as in `parse_hookimpl_opts`.
This deprecated path also calls `cast(...)` without defining it, so it will raise `NameError` when invoked. Please apply the same local `cast` definition here as in `parse_hookimpl_opts` to preserve hookspec compatibility.
</issue_to_address>
### Comment 3
<location path="src/pluggy/_manager.py" line_range="165" />
<code_context>
- options for items decorated with :class:`HookimplMarker`.
- """
- method: object = getattr(plugin, name)
+ def _read_hookimpl_configuration(
+ self, plugin: _Plugin, name: str
+ ) -> HookimplConfiguration | None:
</code_context>
<issue_to_address>
**issue (complexity):** Consider collapsing the new `_read_*` and `_discover_*` helpers into a single `_get_*_configuration` per type and reworking `parse_*_opts` as pure legacy mapping helpers to simplify the discovery flow and avoid redundant config↔mapping round-trips.
You can remove a layer of indirection and the config↔mapping round‑trip by collapsing `_read_*` + `_discover_*` into a single internal helper per type and making the deprecated `parse_*_opts` a pure legacy mapping helper.
### 1. Collapse `_read_*` and `_discover_*` into a single configuration helper
Instead of `_read_hookimpl_configuration` + `_discover_hookimpl_configuration`, use a single `_get_hookimpl_configuration` that:
- Reads the marker attribute.
- Accepts both `HookimplConfiguration` and mapping markers.
- Only calls legacy `parse_hookimpl_opts` if a subclass overrides it.
```python
def _get_hookimpl_configuration(
self, plugin: _Plugin, name: str
) -> HookimplConfiguration | None:
try:
method: object = getattr(plugin, name)
except Exception:
return None
if not inspect.isroutine(method):
return None
try:
attr: object = getattr(method, self.project_name + "_impl", None)
except Exception: # pragma: no cover
attr = None
if isinstance(attr, HookimplConfiguration):
return attr
if isinstance(attr, Mapping):
return hookimpl_config_from_mapping(attr)
# Legacy path: only if subclass overrides parse_hookimpl_opts
if type(self).parse_hookimpl_opts is not PluginManager.parse_hookimpl_opts:
legacy = self.parse_hookimpl_opts(plugin, name)
if isinstance(legacy, Mapping):
return hookimpl_config_from_mapping(legacy)
return None
```
Then `register` becomes:
```python
for name in dir(plugin):
hookimpl_config = self._get_hookimpl_configuration(plugin, name)
if hookimpl_config is not None:
method: _HookImplFunction[object] = getattr(plugin, name)
hookimpl = HookImpl(plugin, plugin_name, method, hookimpl_config)
name = hookimpl_config.specname or name
...
```
Apply the same pattern for hookspecs:
```python
def _get_hookspec_configuration(
self, module_or_class: _Namespace, name: str
) -> HookspecConfiguration | None:
try:
method = getattr(module_or_class, name)
except Exception:
return None
try:
attr: object = getattr(method, self.project_name + "_spec", None)
except Exception: # pragma: no cover
attr = None
if isinstance(attr, HookspecConfiguration):
return attr
if isinstance(attr, Mapping):
return hookspec_config_from_mapping(attr)
if type(self).parse_hookspec_opts is not PluginManager.parse_hookspec_opts:
legacy = self.parse_hookspec_opts(module_or_class, name)
if isinstance(legacy, Mapping):
return hookspec_config_from_mapping(legacy)
return None
```
And `add_hookspecs` uses `_get_hookspec_configuration` directly.
This keeps all behavior (including the override check) but removes the tight coupling between `_read_*` and `_discover_*` and makes the registration/spec discovery path a single function per type.
### 2. Make `parse_*_opts` purely legacy mapping helpers (no round‑trip)
The deprecated `parse_*_opts` can operate directly on the marker attribute as mappings, without calling the internal configuration helper. That removes the config→mapping→config round‑trip when subclasses override them.
```python
def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None:
"""Return legacy dict-shaped hookimpl options, if any.
.. deprecated::
Thin pytest/support concession. Prefer marker-attached configuration
objects; core registration uses `_get_hookimpl_configuration`.
"""
try:
method: object = getattr(plugin, name)
except Exception:
return None
if not inspect.isroutine(method):
return None
try:
attr: object = getattr(method, self.project_name + "_impl", None)
except Exception: # pragma: no cover
return None
if isinstance(attr, Mapping):
return cast(HookimplOpts, attr)
if isinstance(attr, HookimplConfiguration):
# Still support config objects when someone calls this explicitly.
return cast(HookimplOpts, hookimpl_config_to_mapping(attr))
return None
```
```python
def parse_hookspec_opts(
self, module_or_class: _Namespace, name: str
) -> HookspecOpts | None:
"""Return legacy dict-shaped hookspec options, if any.
.. deprecated::
Thin pytest/support concession. Prefer marker-attached configuration
objects; core discovery uses `_get_hookspec_configuration`.
"""
try:
method = getattr(module_or_class, name)
except Exception:
return None
try:
attr: object = getattr(method, self.project_name + "_spec", None)
except Exception: # pragma: no cover
return None
if isinstance(attr, Mapping):
return cast(HookspecOpts, attr)
if isinstance(attr, HookspecConfiguration):
return cast(HookspecOpts, hookspec_config_to_mapping(attr))
return None
```
With this:
- Core discovery/registration uses a single `_get_*_configuration` per type.
- Legacy `parse_*_opts` stays available for pytest/support and subclass overrides, but is clearly separated from the main path and doesn’t participate in config↔mapping round‑trips.
- The override detection is still there, but the control flow is simpler: one internal function per type, one public legacy helper.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| config = self._read_hookimpl_configuration(plugin, name) | ||
| if config is None: | ||
| return None | ||
| return cast(HookimplOpts, hookimpl_config_to_mapping(config)) |
There was a problem hiding this comment.
issue (bug_risk): Using cast here will raise at runtime unless cast is imported in this module.
cast must be imported (e.g., from typing import cast) for this call to work; otherwise parse_hookimpl_opts will raise a NameError at runtime in this compatibility path.
| config = self._read_hookspec_configuration(module_or_class, name) | ||
| if config is None: | ||
| return None | ||
| return cast(HookspecOpts, hookspec_config_to_mapping(config)) |
There was a problem hiding this comment.
issue (bug_risk): Same cast runtime issue in parse_hookspec_opts as in parse_hookimpl_opts.
This deprecated path also calls cast(...) without defining it, so it will raise NameError when invoked. Please apply the same local cast definition here as in parse_hookimpl_opts to preserve hookspec compatibility.
| options for items decorated with :class:`HookimplMarker`. | ||
| """ | ||
| method: object = getattr(plugin, name) | ||
| def _read_hookimpl_configuration( |
There was a problem hiding this comment.
issue (complexity): Consider collapsing the new _read_* and _discover_* helpers into a single _get_*_configuration per type and reworking parse_*_opts as pure legacy mapping helpers to simplify the discovery flow and avoid redundant config↔mapping round-trips.
You can remove a layer of indirection and the config↔mapping round‑trip by collapsing _read_* + _discover_* into a single internal helper per type and making the deprecated parse_*_opts a pure legacy mapping helper.
1. Collapse _read_* and _discover_* into a single configuration helper
Instead of _read_hookimpl_configuration + _discover_hookimpl_configuration, use a single _get_hookimpl_configuration that:
- Reads the marker attribute.
- Accepts both
HookimplConfigurationand mapping markers. - Only calls legacy
parse_hookimpl_optsif a subclass overrides it.
def _get_hookimpl_configuration(
self, plugin: _Plugin, name: str
) -> HookimplConfiguration | None:
try:
method: object = getattr(plugin, name)
except Exception:
return None
if not inspect.isroutine(method):
return None
try:
attr: object = getattr(method, self.project_name + "_impl", None)
except Exception: # pragma: no cover
attr = None
if isinstance(attr, HookimplConfiguration):
return attr
if isinstance(attr, Mapping):
return hookimpl_config_from_mapping(attr)
# Legacy path: only if subclass overrides parse_hookimpl_opts
if type(self).parse_hookimpl_opts is not PluginManager.parse_hookimpl_opts:
legacy = self.parse_hookimpl_opts(plugin, name)
if isinstance(legacy, Mapping):
return hookimpl_config_from_mapping(legacy)
return NoneThen register becomes:
for name in dir(plugin):
hookimpl_config = self._get_hookimpl_configuration(plugin, name)
if hookimpl_config is not None:
method: _HookImplFunction[object] = getattr(plugin, name)
hookimpl = HookImpl(plugin, plugin_name, method, hookimpl_config)
name = hookimpl_config.specname or name
...Apply the same pattern for hookspecs:
def _get_hookspec_configuration(
self, module_or_class: _Namespace, name: str
) -> HookspecConfiguration | None:
try:
method = getattr(module_or_class, name)
except Exception:
return None
try:
attr: object = getattr(method, self.project_name + "_spec", None)
except Exception: # pragma: no cover
attr = None
if isinstance(attr, HookspecConfiguration):
return attr
if isinstance(attr, Mapping):
return hookspec_config_from_mapping(attr)
if type(self).parse_hookspec_opts is not PluginManager.parse_hookspec_opts:
legacy = self.parse_hookspec_opts(module_or_class, name)
if isinstance(legacy, Mapping):
return hookspec_config_from_mapping(legacy)
return NoneAnd add_hookspecs uses _get_hookspec_configuration directly.
This keeps all behavior (including the override check) but removes the tight coupling between _read_* and _discover_* and makes the registration/spec discovery path a single function per type.
2. Make parse_*_opts purely legacy mapping helpers (no round‑trip)
The deprecated parse_*_opts can operate directly on the marker attribute as mappings, without calling the internal configuration helper. That removes the config→mapping→config round‑trip when subclasses override them.
def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None:
"""Return legacy dict-shaped hookimpl options, if any.
.. deprecated::
Thin pytest/support concession. Prefer marker-attached configuration
objects; core registration uses `_get_hookimpl_configuration`.
"""
try:
method: object = getattr(plugin, name)
except Exception:
return None
if not inspect.isroutine(method):
return None
try:
attr: object = getattr(method, self.project_name + "_impl", None)
except Exception: # pragma: no cover
return None
if isinstance(attr, Mapping):
return cast(HookimplOpts, attr)
if isinstance(attr, HookimplConfiguration):
# Still support config objects when someone calls this explicitly.
return cast(HookimplOpts, hookimpl_config_to_mapping(attr))
return Nonedef parse_hookspec_opts(
self, module_or_class: _Namespace, name: str
) -> HookspecOpts | None:
"""Return legacy dict-shaped hookspec options, if any.
.. deprecated::
Thin pytest/support concession. Prefer marker-attached configuration
objects; core discovery uses `_get_hookspec_configuration`.
"""
try:
method = getattr(module_or_class, name)
except Exception:
return None
try:
attr: object = getattr(method, self.project_name + "_spec", None)
except Exception: # pragma: no cover
return None
if isinstance(attr, Mapping):
return cast(HookspecOpts, attr)
if isinstance(attr, HookspecConfiguration):
return cast(HookspecOpts, hookspec_config_to_mapping(attr))
return NoneWith this:
- Core discovery/registration uses a single
_get_*_configurationper type. - Legacy
parse_*_optsstays available for pytest/support and subclass overrides, but is clearly separated from the main path and doesn’t participate in config↔mapping round‑trips. - The override detection is still there, but the control flow is simpler: one internal function per type, one public legacy helper.
There was a problem hiding this comment.
Pull request overview
This PR migrates pluggy’s hook option representation from legacy dict/TypedDict shapes to dedicated configuration objects, while preserving compatibility paths needed by pytest and other legacy integrations.
Changes:
- Introduces
HookspecConfiguration/HookimplConfigurationand updates markers to attach these objects to functions. - Updates core registration/discovery paths (
PluginManager.register/add_hookspecs,HookCaller,HookImpl,HookSpec) to consume configuration objects, with mapping shims for legacy encodings. - Adds a pytest-compat module for legacy TypedDict typing and expands tests/docs to cover the new configuration-based API and migration behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| testing/test_hookcaller.py | Updates tests to assert configuration via attribute-style access instead of dict indexing. |
| testing/test_details.py | Adjusts a subclass override test for legacy parse_hookimpl_opts behavior. |
| testing/test_configuration.py | Adds dedicated tests for configuration classes, mapping shims, and discovery/override behavior. |
| src/pluggy/_pytest_compat.py | Adds legacy TypedDict definitions and mapping conversion helpers for pytest/support. |
| src/pluggy/_manager.py | Switches hook registration/spec discovery to configuration objects with private _read_*/_discover_* helpers and legacy fallback. |
| src/pluggy/_implementation.py | Updates HookImpl to store and expose configuration via object attributes. |
| src/pluggy/_hooks.py | Updates exports/imports to reflect configuration-object API surface. |
| src/pluggy/_decorators.py | Updates markers to attach configuration objects; updates HookSpec to read config attributes. |
| src/pluggy/_config.py | Replaces TypedDict option containers with final configuration classes plus mapping shims. |
| src/pluggy/_caller.py | Updates HookCaller to use configuration objects for historic/firstresult and temp hookimpl opts. |
| src/pluggy/init.py | Exposes the new configuration classes publicly; keeps legacy TypedDicts importable. |
| docs/index.rst | Updates documentation to describe configuration objects and deprecated legacy parsing methods. |
| docs/api_reference.rst | Updates API docs to reference configuration classes instead of TypedDict option types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
334
to
337
| if val: | ||
| assert he_myhook1.example_impl.get(name) | ||
| assert getattr(he_myhook1.example_impl, name) | ||
| else: | ||
| assert not hasattr(he_myhook1, name) |
This was referenced Jul 24, 2026
refactor(caller): Protocol HookCaller, split callers, CompletionHook multicall
pytest-dev/pluggy#708
Open
RonnyPfannschmidt
force-pushed
the
refactor/split-hook-modules
branch
from
July 24, 2026 16:53
8c3f2f7 to
7489fdb
Compare
RonnyPfannschmidt
force-pushed
the
refactor/configuration-objects
branch
from
July 24, 2026 16:53
65ee170 to
6993f10
Compare
RonnyPfannschmidt
force-pushed
the
refactor/split-hook-modules
branch
from
August 12, 2026 10:50
7489fdb to
87bccf2
Compare
RonnyPfannschmidt
force-pushed
the
refactor/configuration-objects
branch
from
August 12, 2026 10:50
6993f10 to
035b254
Compare
RonnyPfannschmidt
force-pushed
the
refactor/split-hook-modules
branch
from
August 12, 2026 10:57
87bccf2 to
f795cf2
Compare
RonnyPfannschmidt
force-pushed
the
refactor/configuration-objects
branch
from
August 12, 2026 10:57
035b254 to
ad3a5a9
Compare
Markers attach HookspecConfiguration/HookimplConfiguration objects. Registration discovers those privately; parse_hookimpl_opts and parse_hookspec_opts remain a deprecated pytest concession that returns legacy dicts and is only called when a subclass overrides them and no modern configuration attribute was found. Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Cursor Grok 4.5 <grok@cursor.com>
RonnyPfannschmidt
force-pushed
the
refactor/split-hook-modules
branch
from
August 12, 2026 11:14
f795cf2 to
fcece74
Compare
RonnyPfannschmidt
force-pushed
the
refactor/configuration-objects
branch
from
August 12, 2026 11:14
ad3a5a9 to
eef72c1
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
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 2 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#704.
Merges happen upstream one step at a time, bottom-up. When step 2 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-submitterSummary
HookspecConfiguration/HookimplConfiguration; markers attach these objects_discover_*/_read_*own registration discoveryparse_hookimpl_opts/parse_hookspec_optskept as a deprecated pytest concession (legacy dicts), only invoked when a subclass overrides them and no modern config attribute was found_pytest_compatfor pytest typingStacked on
refactor/split-hook-modulesfor reviewable incremental diff.Test plan
uv run pytest(164 passed)uv run pre-commit run -aMade with Cursor
Summary by Sourcery
Replace legacy dict-based hook option encodings with configuration objects and preserve compatibility for pytest and other legacy callers.
New Features:
Enhancements:
Tests:
Chores: