Skip to content

chore(structure): split hook modules into role-specific files - #6

Open
RonnyPfannschmidt wants to merge 26 commits into
mainfrom
refactor/split-hook-modules
Open

chore(structure): split hook modules into role-specific files#6
RonnyPfannschmidt wants to merge 26 commits into
mainfrom
refactor/split-hook-modules

Conversation

@RonnyPfannschmidt

@RonnyPfannschmidt RonnyPfannschmidt commented Jul 24, 2026

Copy link
Copy Markdown
Owner

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.

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

  • Factor hook specification/implementation configuration types into a dedicated _config module.
  • Extract hook markers, specifications, and helper utilities into a new _decorators module.
  • Extract hook caller and relay logic into a new _caller module.
  • Extract hook implementation representation into a new _implementation module.
  • Extract multicall execution and wrapper teardown handling into a new _execution module.
  • Update _hooks and _callers to act as thin compatibility layers that re-export symbols from the new modules.

@sourcery-ai

sourcery-ai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Reviewer's Guide

Refactors 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

Change Details Files
Split monolithic _hooks.py into role-focused modules and turn _hooks.py into a compatibility re-export layer.
  • Remove inlined hook types, markers, caller, spec, and implementation classes from _hooks.py.
  • Introduce new modules for configuration, decorators/specs, callers/relay, implementations, and execution.
  • Update _hooks.py to import from the new modules and define a consolidated all for the legacy import path.
src/pluggy/_hooks.py
src/pluggy/_config.py
src/pluggy/_decorators.py
src/pluggy/_caller.py
src/pluggy/_implementation.py
src/pluggy/_execution.py
Extract hook execution (multicall and wrapper machinery) into a dedicated _execution module and keep _callers.py as a thin re-export shim.
  • Move Teardown type alias, run_old_style_hookwrapper, _raise_wrapfail, _warn_teardown_exception, and _multicall into _execution.py.
  • Simplify _callers.py to import the execution helpers from _execution and re-export them via all.
  • Preserve existing behaviour and signatures while only changing where functionality lives.
src/pluggy/_callers.py
src/pluggy/_execution.py

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

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
"""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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

dependabot Bot and others added 22 commits July 27, 2026 21:31
…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)
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
RonnyPfannschmidt force-pushed the refactor/split-hook-modules branch 2 times, most recently from 87bccf2 to f795cf2 Compare August 12, 2026 10:57
RonnyPfannschmidt and others added 4 commits August 12, 2026 13:07
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>
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.

3 participants