fix(docs): qualify the ambiguous Type refs in the foundation rst tree by JarryShaw · Pull Request #714 · JarryShaw/PyPCAPKit

JarryShaw · 2026-09-23T17:30:13Z

Summary

Sphinx defines several classes named Type across pcapkit (including pcapkit/const/l2tp/type.py:18's class Type(IntEnum)), so a bare Type in a hand-written :type: field is ambiguous. Worse, it doesn't just warn — it silently resolves to the wrong target. Confirmed by a full docs build: engine.rst's Type[EngineBase] linked to pcapkit.const.l2tp.type.Type (the L2TP enum) instead of typing.Type.

This fixes the four remaining bare-Type :type: fields in the docs/source tree:

docs/source/pcapkit/foundation/engines/engine.rst:40
docs/source/pcapkit/foundation/reassembly/reassembly.rst:33
docs/source/pcapkit/foundation/reassembly/reassembly.rst:43
docs/source/pcapkit/foundation/traceflow/traceflow.rst:40

Which form was chosen, and why

There are two conventions in this repo: pcapkit/**/*.py's #: autodoc comments use ~typing.Type[...] (tilde, short display text), while docs/source/pcapkit/foundation/traceflow/traceflow.rst:30 already reads typing.Type[pcapkit.protocols.protocol.ProtocolBase] — fully qualified, no tilde.

I defaulted to the .rst tree's own precedent (fully qualified, no tilde), since that's the established local convention for hand-written :type: fields, and one of the four sites being fixed is in the very file that already uses it. The inner type parameter (EngineBase, Protocol, ReassemblyBase, TraceFlowBase) was left unqualified, matching how traceflow.rst:40's own TraceFlowBase and reassembly.rst's ReassemblyBase/Protocol were already written elsewhere in these same files — only the ambiguous Type needed a qualifier. This is a judgement call; the maintainer may prefer the ~typing.Type tilde form instead, since that's what the .py autodoc comments use.

Verification -- the warning diff, not just the exit code

Built with PCAPKIT_SPHINX=1 python -m sphinx -b html docs/source docs/build/html (no -W), once on pristine main and once with this change, and diffed the warnings.

Baseline (pristine main, 699f757): 5 occurrences of more than one target found for cross-reference 'Type' — 4 at the sites this PR fixes, plus 1 at pcapkit/foundation/traceflow/traceflow.py's _foutio #: comment (line 406), which belongs to the still-open #712 and is untouched here.

Fixed: 1 occurrence — only the _foutio site remains (out of scope, owned by #712). The 4 targeted warnings are gone, no new warnings of any kind appeared (verified with a full sorted diff of every WARNING: line between the two builds).

HTML anchor check -- confirmed the links now resolve to the right targets, not just quiet:

Site	Before (`Type` href)	After (`Type` href)
engine.rst:40	`../../const/l2tp.html#pcapkit.const.l2tp.type.Type` (wrong)	`https://docs.python.org/3/library/typing.html#typing.Type` (correct)
reassembly.rst:33	same wrong L2TP target	correct `typing.Type`
reassembly.rst:43	same wrong L2TP target	correct `typing.Type`
traceflow.rst:40	same wrong L2TP target	correct `typing.Type`

The inner class references (EngineBase, Protocol, ReassemblyBase, TraceFlowBase) all resolve to their correct internal targets in both builds and were unaffected by this change.

Not part of #709, left alone: the same signatures also show dict resolving to pcapkit.const.reg.apptype.AppType.dict instead of the builtin -- a separate, pre-existing ambiguity bug unrelated to Type, present in both the before and after builds. Out of scope here.

Test coverage

No test added. tests/project/test_documentation_claims.py's module docstring states the project's explicit policy on exactly this category:

Deliberately not tested here either: whether a Sphinx cross-reference resolves. That is a property of the built inventory, not of the source, and the honest check for it is a nitpicky sphinx-build -- some 20 minutes, and it needs the docs toolchain installed. A unit test that re-implemented Sphinx's resolution rules would pass while the real build failed, which is worse than not testing it. The measured counts are recorded in the pull request instead.

That is exactly this change, so the measured counts above are the test. Ran the existing tests/project/test_documentation_claims.py (3 tests) under coverage as a sanity check -- still passes unchanged, since this is a doc-only change with no pcapkit/ lines touched.

Scope note -- Closes vs. Part of

Grepped both pcapkit/ and docs/source/ for remaining bare Type[ sites. Two remain, both in pcapkit/foundation/traceflow/traceflow.py (#: comments at lines 146 and 406) -- owned by the still-open #712, which this PR does not touch per instructions. So this is Part of #709, not a close.

Closes: N/A (Part of #709)

Sphinx defines several classes named Type across pcapkit (including pcapkit.const.l2tp.type.Type), so a bare `Type` in a hand-written :type: field is ambiguous and resolves silently to the wrong one -- confirmed by a docs build showing engine.rst's `Type[EngineBase]` linking to the L2TP enum instead of typing.Type. Qualify the four remaining bare-Type :type: fields in the docs/source tree to `typing.Type[...]`, matching the fully-qualified form already used at traceflow.rst:30: - docs/source/pcapkit/foundation/engines/engine.rst:40 - docs/source/pcapkit/foundation/reassembly/reassembly.rst:33,43 - docs/source/pcapkit/foundation/traceflow/traceflow.rst:40 Two more bare-Type sites remain in pcapkit/foundation/traceflow/ traceflow.py's `#:` comments (lines 146 and 406); those belong to the still-open #712 and are left untouched here. Part of #709. Verified with a full `make docs` warning diff: the four "more than one target found for cross-reference 'Type'" warnings drop to zero with no new warnings, and the built HTML now links `Type` to typing.Type instead of the L2TP enum.

JarryShaw · 2026-09-23T18:11:57Z

Cross-review verdict: GOOD TO GO (independent review on a different model, read-only, ran its own before/after docs builds in a separate scratch checkout).

Full cross-review notes

Verified firsthand by rebuilding both pristine main (699f757) and this PR (82e3258) in isolated scratch clones:

Baseline: 5 more than one target found for cross-reference 'Type' warnings (the 4 sites this PR fixes + TraceFlow._foutio, owned by fix(traceflow): qualify the bare Type[Dumper] autodoc references (#709) #712). After: 1 (_foutio only). comm diff of the full sorted warning lists confirms exactly those 4 lines disappeared and nothing else changed (56 -> 52 WARNING: lines).
HTML anchor check confirms real re-resolution, not silencing: engine.html's Type link moves from ../../const/l2tp.html#pcapkit.const.l2tp.type.Type (wrong) to https://docs.python.org/3/library/typing.html#typing.Type (correct).
The rendered page never shows typing. regardless of whether the source says typing.Type or ~typing.Type -- Sphinx shortens the display for this kind of annotation-parsed :type: field. So the no-tilde form chosen here is stylistically moot and matches the repo's existing typing.Type[ precedents (traceflow.rst:30, infoclass.rst:41, protocols/index.rst:242) -- zero ~ in any rst :type: field.
Grep confirms no remaining bare Type[ in any .rst file; the two remaining #:-comment sites are both in traceflow.py (lines 146, 406), owned by the still-open fix(traceflow): qualify the bare Type[Dumper] autodoc references (#709) #712, no overlap with this PR's files. "Part of docs: bare 'Type' in TraceFlow._foutio's autodoc comment is ambiguous across five classes, adding a Sphinx cross-reference warning #709" is the right framing.
Diff is exactly 3 files, +4/-4, docs-only, one commit on top of current main.
Minor note for whoever picks up fix(traceflow): qualify the bare Type[Dumper] autodoc references (#709) #712: of its two sites, only _foutio (line 406) is actually broken (renders pointing at the L2TP enum); __output__ (line 146) already resolves correctly to typing.Type because its type comes from a real resolved annotation rather than free-text.
The dict -> pcapkit.const.reg.apptype.AppType.dict mis-resolution on these same signatures is confirmed pre-existing in both builds -- unrelated to Type, correctly left untouched here.

No disputes.

… new entries Cross-review on issuecomment-5800589100 (PR #657) found the eight new 1.5.0 entries wrong in several places, spot-checked against source before each fix: - #709 entry: dropped the false "no PR yet filed" clause -- PR #714, opened five minutes before the commit claiming otherwise, covers exactly the four sites named. Corrected the inertness explanation for line 146: the real ``# type:`` comment is at line 162 (sixteen lines below, not two), it also carries a bare ``Type[Dumper]``, and the likely mechanism is ``sphinx_autodoc_typehints`` rather than ``napoleon_attr_annotations``. - #702 entry: "six of seven guard lines" is seven of seven -- #677's sweep exempts nothing and reaches ``tcp/flags.py`` too. Split the conflated claim about "one tuple ending in 65536 swept every registry" into the two tests that actually exist. Reworded the "seven registries are not, at four distinct widths" line, which reads as self-contradicting two of its own examples. - #684 entry: "one write-up apiece" -> one unified write-up; "every automodule" -> there are zero, the exclusion works through ``autoclass`` and ``autodoc_default_options``; "built-in and third-party engines' own _backend" -> only the third-party ones gained it, built-in gained a different attribute set. Also fixed the "bare-\n``Type``" line wrap that renders as "bare- `Type`" -- the only trailing-hyphen wrap in the file. - #708 entry: only one of the two tests re-derives against git; the sibling still compares the module to itself. Dropped "today's six tracked names", which contradicts the #685 entry naming 2. - #700 entry: only ``_tiers.py`` cited "six"; ``test_tier_guard.py`` said "seventh capture" instead. Two smaller imprecisions, fixed as flagged: the #672/#679 entry's "both codes reach this class" now says packing explicitly (parsing only reaches 129, per #690, already stated later in the same entry), and its claim of a pre-fix ``R1_COUNTER`` override is corrected to a schema default. The #685 entry's "shown failing" is now qualified to the 2 of 5 tests (5 of 8 subtests) that actually fail against the pre-change tree. Checked but not changed: no entry in the file asserts the "issue numbers only" citation convention the review tested, so there was nothing to correct there; the five inline PR references it flagged as legitimate are untouched. Verified: docutils 0.22.4 ``publish_doctree`` over the changed block -- zero system messages, one bullet_list of 8 items.

A third cross-review found the same failure mode again: the last pass replaced vague prose with false prose. This round deletes rather than corrects. - #702 entry: dropped the attribution of the seven guard raise lines to specific PRs. "the four mh.* guards came from #632" was false and self-contradicts the file's own #623 entry three sections earlier ("The range guard above it is untouched" -- a05f461's own commit message says the same). The entry does not need to say which PR added which guard; it only needs to say the lines were already covered. - #709 entry: dropped the `TraceFlowBase.__dict__['__annotations__']` citation. Measured: that lookup raises KeyError, not "is empty" -- `TraceFlowBase.__annotations__` (the resolved, non-dict-shortcut form) is `{'__cached__': ...}`, non-empty. The conclusion (`__output__` is unannotated) holds, but the cited evidence does not, so it's dropped rather than replaced with a corrected measurement. - #709 entry: rewritten to the present tense of a merged world. #709 closed once #712 landed (the two traceflow.py sites this entry describes); the four docs/*.rst sites are fixed separately by #714, also merged. No more "does not close" / "stays open" hedging. Refs #657

…tale tense Round 5: fixed the two defects the fourth cross-review found at head 93a4126 (NEEDS CHANGES). - `:2389` -- "This pair of sites is what closes #709" was false: #712 and #714 both state in their own bodies that they are `Part of #709`, not a close, and #709's closed timeline event carries `commit_id: null` (a manual close). Made non-causal: "This pair of sites is part of #709." - `:2325` -- "#709 tracks it" was present tense describing an issue that is now closed, contradicting the #709 entry itself. Changed to past tense: "#709 tracked it." Regenerated CHANGELOG.md from the edited entry. | check | result | |---|---| | `changelog_md.py --check` | exit 0 | | `pytest tests/project/test_changelog_md.py -q` | 47 passed, 37 subtests |

… new entries Cross-review on issuecomment-5800589100 (PR #657) found the eight new 1.5.0 entries wrong in several places, spot-checked against source before each fix: - #709 entry: dropped the false "no PR yet filed" clause -- PR #714, opened five minutes before the commit claiming otherwise, covers exactly the four sites named. Corrected the inertness explanation for line 146: the real ``# type:`` comment is at line 162 (sixteen lines below, not two), it also carries a bare ``Type[Dumper]``, and the likely mechanism is ``sphinx_autodoc_typehints`` rather than ``napoleon_attr_annotations``. - #702 entry: "six of seven guard lines" is seven of seven -- #677's sweep exempts nothing and reaches ``tcp/flags.py`` too. Split the conflated claim about "one tuple ending in 65536 swept every registry" into the two tests that actually exist. Reworded the "seven registries are not, at four distinct widths" line, which reads as self-contradicting two of its own examples. - #684 entry: "one write-up apiece" -> one unified write-up; "every automodule" -> there are zero, the exclusion works through ``autoclass`` and ``autodoc_default_options``; "built-in and third-party engines' own _backend" -> only the third-party ones gained it, built-in gained a different attribute set. Also fixed the "bare-\n``Type``" line wrap that renders as "bare- `Type`" -- the only trailing-hyphen wrap in the file. - #708 entry: only one of the two tests re-derives against git; the sibling still compares the module to itself. Dropped "today's six tracked names", which contradicts the #685 entry naming 2. - #700 entry: only ``_tiers.py`` cited "six"; ``test_tier_guard.py`` said "seventh capture" instead. Two smaller imprecisions, fixed as flagged: the #672/#679 entry's "both codes reach this class" now says packing explicitly (parsing only reaches 129, per #690, already stated later in the same entry), and its claim of a pre-fix ``R1_COUNTER`` override is corrected to a schema default. The #685 entry's "shown failing" is now qualified to the 2 of 5 tests (5 of 8 subtests) that actually fail against the pre-change tree. Checked but not changed: no entry in the file asserts the "issue numbers only" citation convention the review tested, so there was nothing to correct there; the five inline PR references it flagged as legitimate are untouched. Verified: docutils 0.22.4 ``publish_doctree`` over the changed block -- zero system messages, one bullet_list of 8 items.

A third cross-review found the same failure mode again: the last pass replaced vague prose with false prose. This round deletes rather than corrects. - #702 entry: dropped the attribution of the seven guard raise lines to specific PRs. "the four mh.* guards came from #632" was false and self-contradicts the file's own #623 entry three sections earlier ("The range guard above it is untouched" -- a05f461's own commit message says the same). The entry does not need to say which PR added which guard; it only needs to say the lines were already covered. - #709 entry: dropped the `TraceFlowBase.__dict__['__annotations__']` citation. Measured: that lookup raises KeyError, not "is empty" -- `TraceFlowBase.__annotations__` (the resolved, non-dict-shortcut form) is `{'__cached__': ...}`, non-empty. The conclusion (`__output__` is unannotated) holds, but the cited evidence does not, so it's dropped rather than replaced with a corrected measurement. - #709 entry: rewritten to the present tense of a merged world. #709 closed once #712 landed (the two traceflow.py sites this entry describes); the four docs/*.rst sites are fixed separately by #714, also merged. No more "does not close" / "stays open" hedging. Refs #657

…tale tense Round 5: fixed the two defects the fourth cross-review found at head 93a4126 (NEEDS CHANGES). - `:2389` -- "This pair of sites is what closes #709" was false: #712 and #714 both state in their own bodies that they are `Part of #709`, not a close, and #709's closed timeline event carries `commit_id: null` (a manual close). Made non-causal: "This pair of sites is part of #709." - `:2325` -- "#709 tracks it" was present tense describing an issue that is now closed, contradicting the #709 entry itself. Changed to past tense: "#709 tracked it." Regenerated CHANGELOG.md from the edited entry. | check | result | |---|---| | `changelog_md.py --check` | exit 0 | | `pytest tests/project/test_changelog_md.py -q` | 47 passed, 37 subtests |

… new entries Cross-review on issuecomment-5800589100 (PR #657) found the eight new 1.5.0 entries wrong in several places, spot-checked against source before each fix: - #709 entry: dropped the false "no PR yet filed" clause -- PR #714, opened five minutes before the commit claiming otherwise, covers exactly the four sites named. Corrected the inertness explanation for line 146: the real ``# type:`` comment is at line 162 (sixteen lines below, not two), it also carries a bare ``Type[Dumper]``, and the likely mechanism is ``sphinx_autodoc_typehints`` rather than ``napoleon_attr_annotations``. - #702 entry: "six of seven guard lines" is seven of seven -- #677's sweep exempts nothing and reaches ``tcp/flags.py`` too. Split the conflated claim about "one tuple ending in 65536 swept every registry" into the two tests that actually exist. Reworded the "seven registries are not, at four distinct widths" line, which reads as self-contradicting two of its own examples. - #684 entry: "one write-up apiece" -> one unified write-up; "every automodule" -> there are zero, the exclusion works through ``autoclass`` and ``autodoc_default_options``; "built-in and third-party engines' own _backend" -> only the third-party ones gained it, built-in gained a different attribute set. Also fixed the "bare-\n``Type``" line wrap that renders as "bare- `Type`" -- the only trailing-hyphen wrap in the file. - #708 entry: only one of the two tests re-derives against git; the sibling still compares the module to itself. Dropped "today's six tracked names", which contradicts the #685 entry naming 2. - #700 entry: only ``_tiers.py`` cited "six"; ``test_tier_guard.py`` said "seventh capture" instead. Two smaller imprecisions, fixed as flagged: the #672/#679 entry's "both codes reach this class" now says packing explicitly (parsing only reaches 129, per #690, already stated later in the same entry), and its claim of a pre-fix ``R1_COUNTER`` override is corrected to a schema default. The #685 entry's "shown failing" is now qualified to the 2 of 5 tests (5 of 8 subtests) that actually fail against the pre-change tree. Checked but not changed: no entry in the file asserts the "issue numbers only" citation convention the review tested, so there was nothing to correct there; the five inline PR references it flagged as legitimate are untouched. Verified: docutils 0.22.4 ``publish_doctree`` over the changed block -- zero system messages, one bullet_list of 8 items.

A third cross-review found the same failure mode again: the last pass replaced vague prose with false prose. This round deletes rather than corrects. - #702 entry: dropped the attribution of the seven guard raise lines to specific PRs. "the four mh.* guards came from #632" was false and self-contradicts the file's own #623 entry three sections earlier ("The range guard above it is untouched" -- a05f461's own commit message says the same). The entry does not need to say which PR added which guard; it only needs to say the lines were already covered. - #709 entry: dropped the `TraceFlowBase.__dict__['__annotations__']` citation. Measured: that lookup raises KeyError, not "is empty" -- `TraceFlowBase.__annotations__` (the resolved, non-dict-shortcut form) is `{'__cached__': ...}`, non-empty. The conclusion (`__output__` is unannotated) holds, but the cited evidence does not, so it's dropped rather than replaced with a corrected measurement. - #709 entry: rewritten to the present tense of a merged world. #709 closed once #712 landed (the two traceflow.py sites this entry describes); the four docs/*.rst sites are fixed separately by #714, also merged. No more "does not close" / "stays open" hedging. Refs #657

…tale tense Round 5: fixed the two defects the fourth cross-review found at head 93a4126 (NEEDS CHANGES). - `:2389` -- "This pair of sites is what closes #709" was false: #712 and #714 both state in their own bodies that they are `Part of #709`, not a close, and #709's closed timeline event carries `commit_id: null` (a manual close). Made non-causal: "This pair of sites is part of #709." - `:2325` -- "#709 tracks it" was present tense describing an issue that is now closed, contradicting the #709 entry itself. Changed to past tense: "#709 tracked it." Regenerated CHANGELOG.md from the edited entry. | check | result | |---|---| | `changelog_md.py --check` | exit 0 | | `pytest tests/project/test_changelog_md.py -q` | 47 passed, 37 subtests |

JarryShaw added bug docs Pull requests that change documentation only (docs: subject prefix) labels Sep 23, 2026

This was referenced Sep 23, 2026

docs: bare 'dict' in :type: fields resolves to AppType.dict, an IANA service enum member, not the builtin #717

Closed

docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

Open

JarryShaw mentioned this pull request Sep 23, 2026

CI: cut job count and duplicated work — contention, not job duration, is what makes CI slow #715

Closed

JarryShaw merged commit b565880 into main Sep 23, 2026
25 of 26 checks passed

JarryShaw deleted the fix/709-rst-type-xrefs branch September 23, 2026 21:10

JarryShaw mentioned this pull request Sep 23, 2026

fix(docs): stop AppType shadowing six builtins in cross-references #724

Closed

5 tasks

JarryShaw removed the bug label Sep 23, 2026

Navigation Menu

Uh oh!

fix(docs): qualify the ambiguous Type refs in the foundation rst tree - #714

fix(docs): qualify the ambiguous Type refs in the foundation rst tree#714
JarryShaw merged 1 commit into
mainfrom
fix/709-rst-type-xrefs

JarryShaw commented Sep 23, 2026

Uh oh!

JarryShaw commented Sep 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

JarryShaw commented Sep 23, 2026

Summary

Which form was chosen, and why

Verification -- the warning diff, not just the exit code

Test coverage

Scope note -- Closes vs. Part of

Uh oh!

JarryShaw commented Sep 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant