chore(structure): split hook modules into role-specific files - #6
Open
RonnyPfannschmidt wants to merge 26 commits into
Open
chore(structure): split hook modules into role-specific files#6RonnyPfannschmidt wants to merge 26 commits into
RonnyPfannschmidt wants to merge 26 commits into
Conversation
Reviewer's GuideRefactors pluggy’s internal hook machinery into role-specific modules while preserving the original public surface via re-export shims for backward compatibility. 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 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/pluggy/_caller.py" line_range="271" />
<code_context>
+ """A proxy to another HookCaller which manages calls to all registered
+ plugins except the ones from remove_plugins."""
+
+ # This class is unusual: in inhertits from `HookCaller` so all of
+ # the *code* runs in the class, but it delegates all underlying *data*
+ # to the original HookCaller.
</code_context>
<issue_to_address>
**nitpick (typo):** Fix typo in comment: "inhertits" should be "inherits".
Please fix the spelling here so the comment remains clear for future maintainers, given it documents a subtle design choice.
```suggestion
# This class is unusual: it inherits from `HookCaller` so all of
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| """A proxy to another HookCaller which manages calls to all registered | ||
| plugins except the ones from remove_plugins.""" | ||
|
|
||
| # This class is unusual: in inhertits from `HookCaller` so all of |
There was a problem hiding this comment.
nitpick (typo): Fix typo in comment: "inhertits" should be "inherits".
Please fix the spelling here so the comment remains clear for future maintainers, given it documents a subtle design choice.
Suggested change
| # This class is unusual: in inhertits from `HookCaller` so all of | |
| # This class is unusual: it inherits from `HookCaller` so all of |
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
…ytest-dev#713) Bumps [pypa/gh-action-pypi-publish](https://github.com/pypa/gh-action-pypi-publish) from 1.14.0 to 1.14.1. - [Release notes](https://github.com/pypa/gh-action-pypi-publish/releases) - [Commits](pypa/gh-action-pypi-publish@cef2210...ba38be9) --- updated-dependencies: - dependency-name: pypa/gh-action-pypi-publish dependency-version: 1.14.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/checkout](https://github.com/actions/checkout) from 7.0.0 to 7.0.1. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@9c091bb...3d3c42e) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: 7.0.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…v#711) Bumps [actions/setup-python](https://github.com/actions/setup-python) from 6.3.0 to 7.0.0. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@ece7cb0...5fda3b9) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: 7.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
) Bumps [astral-sh/setup-uv](https://github.com/astral-sh/setup-uv) from 8.3.2 to 9.0.0. - [Release notes](https://github.com/astral-sh/setup-uv/releases) - [Commits](astral-sh/setup-uv@11f9893...c771a70) --- updated-dependencies: - dependency-name: astral-sh/setup-uv dependency-version: 9.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…not-found path Ran the suite under coverage.py and found two real gaps in otherwise near-100% covered code. The module-level __getattr__ that lazily resolves pluggy.__version__ via importlib.metadata (added per changelog/590.trivial.rst) had no test exercising either branch. And HookCaller._remove_plugin's ValueError, raised when asked to remove a plugin that never registered on that hook, was untested even though the normal removal path is well covered through PluginManager.unregister.
…ytest-dev#719) Bumps [pypa/gh-action-pypi-publish](https://github.com/pypa/gh-action-pypi-publish) from 1.14.1 to 1.14.2. - [Release notes](https://github.com/pypa/gh-action-pypi-publish/releases) - [Commits](pypa/gh-action-pypi-publish@ba38be9...dc37677) --- updated-dependencies: - dependency-name: pypa/gh-action-pypi-publish dependency-version: 1.14.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
updates: - [github.com/astral-sh/ruff-pre-commit: v0.15.22 → v0.16.2](astral-sh/ruff-pre-commit@v0.15.22...v0.16.2)
for more information, see https://pre-commit.ci
Fixes the flake8-comprehensions C408 violations that ruff 0.16 reports now that it enables the rule by default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
``0 / 0`` as a statement is a deliberate "blow up here", and ``module.x.broken`` a deliberate attribute access, but both read as dead code, which is what flake8-bugbear B018 (enabled by default as of ruff 0.16) reports. Say what is meant instead: raise the error, and assign the attribute access that is only there for its side effect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ruff 0.16 enables flake8-simplify by default: * SIM102 in ``varnames()`` and in a test plugin manager: flatten a nested ``if`` into the branch condition. * SIM103 in the Sphinx warning filter: return the negated condition instead of branching to two literals. * SIM118 in ``HookCaller._verify_all_args_are_provided()``: membership on the mapping itself, no ``.keys()`` needed. Also parenthesizes the ``or``/``and`` condition ruff collapsed in the previous commit, which relied on operator precedence to be read right. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
``from collections.abc import Set`` shadows the reading of the ``set`` builtin at a glance, which is what flake8-pyi PYI025 (default as of ruff 0.16) points out. Alias it to ``AbstractSet``. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The hookimpl loop in ``PluginManager.register()`` reused ``name``, the method's own argument, for two different things: the attribute name being inspected on the plugin, and then the hook name it maps to. Give each its own name, as PLR1704 (default as of ruff 0.16) asks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PERF402, enabled by default as of ruff 0.16. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
``varnames()`` and its benchmark counterpart fetch ``__call__`` to inspect the signature of a callable object. B004 (default as of ruff 0.16) reads that as an unreliable callable test and suggests ``callable()``, which returns a bool and would break both. Mark it, and say why in the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
``test_dist_facade_identity_equality_and_hash`` asserts that a facade equals itself, which is exactly the identity semantics under test. PLR0124 (default as of ruff 0.16) flags the self-comparison. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The multicall engine and ``Result.from_call()`` must capture whatever a hook implementation raised -- BaseException included -- and hand it to the hook wrappers; narrowing those handlers would change pluggy's contract. ``varnames()`` and ``parse_hookimpl_opts()`` likewise read attributes off arbitrary plugin objects, which may raise anything (see ``test_plugin_getattr_raises_errors``). flake8-blind-except is enabled by default as of ruff 0.16 and flags all thirteen sites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
``test_wrapper_exception_chaining`` and ``test_plugin_getattr_raises_errors`` raise plain ``Exception`` deliberately: the point is that pluggy carries and chains exception types it knows nothing about. tryceratops is enabled by default as of ruff 0.16. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ruff already selects E, W and F, and ruff format owns line length, so flake8 only duplicated it -- and the two now disagree: ruff 0.16 enables RUF100, which removed the ``# noqa: E501`` on the documentation URL in ``_warn_teardown_exception()`` because ruff's own E501 does not flag a line that has nothing left to wrap, while flake8's E501 still does. The URL is 93 characters, so no indentation fits it under 99 either. The only hook-specific check lost is flake8-typing-imports, which warns about typing features newer than the minimum Python. pluggy requires 3.10, ruff infers the same target version from ``requires-python``, and mypy runs on every commit, so it had nothing left to catch. This also unpins pre-commit-hooks, which dropped its flake8 hook in v3.0.0 and could not be autoupdated past v2.1.0 while it was in use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit claimed mypy already covered what flake8-typing-imports checked. That was only half true: ``[tool.mypy]`` set no ``python_version``, so mypy checked against whichever interpreter the hook happened to run on -- 3.14 in the pre-commit environment -- and a ``typing`` member added after 3.10 passed unnoticed. Pin it to 3.10, matching ``requires-python``. mypy then reads typeshed's ``sys.version_info`` guards and reports, for instance, ``Module "typing" has no attribute "Self"``, which is what TYP001 used to catch. It covers more than the plugin did, too: every guarded stdlib attribute rather than only ``typing`` imports, and uses as well as import lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e-config [pre-commit.ci] pre-commit autoupdate
…getattr-and-remove-plugin test: cover pluggy.__version__ getattr and HookCaller._remove_plugin …
RonnyPfannschmidt
force-pushed
the
refactor/split-hook-modules
branch
2 times, most recently
from
August 12, 2026 10:57
87bccf2 to
f795cf2
Compare
ruff 0.16 enables flake8-bugbear by default, so the bare ``pluggy.nope`` expression added in pytest-dev#718 fails B018 and turns pre-commit.ci red on main. Bind it to ``_`` like the other attribute-access checks in that test suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…8-dunder-getattr-test lint: fix B018 failure on main
Add the design/ writeup chain (decisions + per-step docs 01-07) that guides the stacked refactoring branches. Exclude design/ from blacken-docs since the docs use elided pseudo-code signatures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Move hook types, markers, callers, implementations, and multicall out of the monolithic _hooks/_callers modules so later typed-config and CompletionHook work can land without thrashing one huge file. Keep _hooks and _callers as re-export shims for import compatibility. 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
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 1 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#703.
Merges happen upstream one step at a time, bottom-up. When step 1 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 01 of the internal-refactoring series (see design/ docs, added in step 02's branch).
Splits _hooks.py into role-specific modules (_config, _decorators, _caller, _implementation, _execution) with _hooks.py kept as a compat re-export layer.
Part of the stacked chain: each PR in this chain targets the previous step's branch so it shows only its own diff. The full-diff PRs against pytest-dev/pluggy link back here.
🤖 Generated with Claude Code
Summary by Sourcery
Split the monolithic internal hooks module into role-specific modules while preserving backward-compatible re-exports.
Enhancements: