Remove unnecessary future annotation imports #2

Open

justine wants to merge 3 commits from fix/future-annotations-audit into main

pull from: fix/future-annotations-audit

merge into: justine:main

justine:main

justine:refactor/kb-rename

justine:chore/outdated-target

Conversation 0 Commits 3 Files changed 13 +87 -41

justine commented 2026-08-16 23:02:46 -07:00

Owner

Copy link

Closes #1

What changed

Audited where from __future__ import annotations is actually required. It is load-bearing in four files and dead weight in the other seven.

Removing it from all eleven broke fifteen tests, which is the finding worth recording: pluggy calls inspect.signature() on hookspecs and hookimpls, and under PEP 649 that resolves the annotations at runtime. A TYPE_CHECKING-only import in a module pluggy inspects therefore raises NameError at registration.

Files	Import	Reason
api/_hooks.py, api/timestamps.py, sources.py, tests/test_plugins.py	Kept, with a comment	pluggy inspects these signatures
The other seven	Removed	Unnecessary under PEP 649 on Python 3.14

Also deselects the ruff FA rule, which would otherwise demand the import everywhere, and adds test_hookspecs_resolve_at_runtime as a guard.

Documents in CLAUDE.md that every change goes on a branch and every PR waits for review.

Filename effect

None. No naming logic changed.

Constraints

• Read commands still mutate nothing
• No behavior change intended; the four required imports are retained

Verification

$ uv run tox
  license: OK
  lint: OK
  format: OK
  typecheck: OK
  pyright: OK
  test: OK   57 passed, 1 skipped
$ uv run tox -e cov
  Required test coverage of 95% reached. Total coverage: 99.27%

The guard test was verified to fail with NameError when the import is stripped from _hooks.py, so it cannot regress silently.

Closes #1 ## What changed Audited where `from __future__ import annotations` is actually required. It is load-bearing in four files and dead weight in the other seven. Removing it from all eleven broke fifteen tests, which is the finding worth recording: **pluggy calls `inspect.signature()` on hookspecs and hookimpls**, and under PEP 649 that resolves the annotations at runtime. A `TYPE_CHECKING`-only import in a module pluggy inspects therefore raises `NameError` at registration. | Files | Import | Reason | |---|---|---| | `api/_hooks.py`, `api/timestamps.py`, `sources.py`, `tests/test_plugins.py` | Kept, with a comment | pluggy inspects these signatures | | The other seven | Removed | Unnecessary under PEP 649 on Python 3.14 | Also deselects the ruff `FA` rule, which would otherwise demand the import everywhere, and adds `test_hookspecs_resolve_at_runtime` as a guard. Documents in CLAUDE.md that every change goes on a branch and every PR waits for review. ## Filename effect None. No naming logic changed. ## Constraints - [x] Read commands still mutate nothing - [x] No behavior change intended; the four required imports are retained ## Verification ```console $ uv run tox license: OK lint: OK format: OK typecheck: OK pyright: OK test: OK 57 passed, 1 skipped $ uv run tox -e cov Required test coverage of 95% reached. Total coverage: 99.27% ``` The guard test was verified to fail with `NameError` when the import is stripped from `_hooks.py`, so it cannot regress silently.

justine added 1 commit 2026-08-16 23:02:46 -07:00

Remove unnecessary future annotation imports ... 9d6c56178d

`from __future__ import annotations` was added to all eleven source and test
files by habit, not need: the ruff `FA` rule came across with archivist's rule
set and was satisfied rather than questioned. This project has been
`requires-python = ">=3.14"` since it was created, and PEP 649 makes
annotations lazy by default there, so PEP 604 unions, PEP 585 generics, and
TYPE_CHECKING-only imports all work without it.

Removing it from all eleven broke fifteen tests, which is the finding worth
recording. pluggy calls `inspect.signature()` on hookspecs and hookimpls, and
in 3.14 that resolves the annotations, so a TYPE_CHECKING-only import in a
module pluggy inspects raises NameError at registration. The import is
therefore load-bearing in exactly four files — the hookspecs, the
TimestampSource base, the built-in hookimpls, and the fake plugins in the
tests — and dead weight in the other seven.

Keep it in those four with a comment explaining why, drop it elsewhere, and
deselect `FA`, which would otherwise demand it everywhere. Add
`test_hookspecs_resolve_at_runtime`, verified to fail with NameError when the
import is stripped from `_hooks.py`, so this cannot regress silently.

Also record in CLAUDE.md that every change goes on a branch and every pull
request waits for review; a green CI run is not approval.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018XhA69mkFRwmkhrDvRsQLW

justine added 1 commit 2026-08-16 23:12:27 -07:00

Cite the upstream bug behind the retained future imports ... 5d3668810d

Ref #1

The previous commit kept `from __future__ import annotations` in four files
with a comment stating that pluggy calls `inspect.signature()` and therefore
requires it. That was sourced from reading pluggy's code, not from any
document, and it framed a temporary bug workaround as permanent design.

pluggy's documentation says nothing about signature inspection or annotation
handling. The actual record is pytest-dev/pluggy#629, "`pluggy` requires
`from __future__ import annotations` although it shouldn't in Python 3.14",
where the maintainer calls it "a partial stdlib bug" — the underlying cause is
`inspect.signature` evaluating annotations rather than requesting the
FORWARDREF format, with pluggy merely the caller. Fix PR #668 requests the
STRING format and is merged to main but unreleased; 1.6.0 predates it.

Rewrite the four comments and the CLAUDE.md section to carry the issue number,
upstream's verdict, the release status, and the condition for deleting the
workaround. When a pluggy release containing #668 lands, all four imports go.

Add a project convention requiring that citation for any code that exists only
to accommodate a dependency: issue, verdict, status, removal condition, or an
explicit note that no upstream discussion was found. "The tests fail without
it" establishes that a workaround is load-bearing today, not that the behavior
is intended or unfixed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018XhA69mkFRwmkhrDvRsQLW

justine added 1 commit 2026-08-16 23:43:55 -07:00

Remove the last four future imports by importing at runtime ... 53646f518b

Ref #1

The previous commits kept `from __future__ import annotations` in the four
modules pluggy inspects, treating it as unavoidable. It was not. Those modules
only needed it because `datetime` and `Path` sat under `if TYPE_CHECKING:`;
importing them normally makes the annotations resolvable and the future import
pointless. Both are stdlib and already loaded, so the runtime cost is nil.

The project now contains no `from __future__ import annotations` at all, and
the workaround needs no revisiting when pluggy releases the fix for #629 —
runtime imports sidestep the bug rather than accommodate it. Keep #629 and
#668 in CLAUDE.md as context for why these particular imports may not move
back under TYPE_CHECKING.

Add `test_hookimpl_signatures_resolve_at_runtime` so the guard covers hookimpls
and not only the hookspecs, and verify both guards fail with NameError when an
import is moved back.

Drop the per-module comments repeating the explanation; the tests enforce it
and CLAUDE.md explains it, so one comment at the hookspec definitions is
enough.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018XhA69mkFRwmkhrDvRsQLW

All checks were successful

Hide checks

PR / issue-link (pull_request) Successful in 1s

Details

CI / check (pull_request) Successful in 1m14s

Details

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/future-annotations-audit:fix/future-annotations-audit

git switch fix/future-annotations-audit

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff fix/future-annotations-audit

git switch fix/future-annotations-audit

git rebase main

git switch main

git merge --ff-only fix/future-annotations-audit

git switch fix/future-annotations-audit

git rebase main

git switch main

git merge --no-ff fix/future-annotations-audit

git switch main

git merge --squash fix/future-annotations-audit

git switch main

git merge --ff-only fix/future-annotations-audit

git switch main

git merge fix/future-annotations-audit

git push origin main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

justine/fname!2

No description provided.