fix(traceflow): qualify the bare Type[Dumper] autodoc references (#709) - #712
Conversation
|
❌ NEEDS CHANGES — The 2-line code fix is verified correct (reproduced the |
Detailed reviewReviewed on Sonnet (author ran on Opus), read-only in a git worktree at Builds run with 1-2. Before/after warning counts — reproduced, but the fix alone is a no-op today
The two warning lists are byte-identical ( 3. Reproducing the
|
| # | Claim | Verdict |
|---|---|---|
| 1-2 | before/after warning diff | Confirmed — but the fix alone changes 0 warnings against real main today (both sites are latent) |
| 3 | _foutio reproduction |
Confirmed, exact warning text reproduced |
| 4 | ~typing.Type resolves, doesn't just silence |
Confirmed, and the pre-fix state is worse than described (wrong link, not just a warning) |
| 5 | 8-file convention | Confirmed, all 8 verified |
| 6 | scope grep = 0 | Confirmed true; no live Protocol/Dumper/Schema collisions found either |
| 7 | "correction to #709" (4 occurrences = these 2 sites elsewhere) | Contradicted — the 4 warnings come from 3 other .rst files, unaffected by this PR |
| 8 | no test needed | Agree, with direct project precedent |
The code diff itself is correct, minimal, and verified to do more good than described (claim 4). The change I'm asking for is to the prose: drop or correct the "correction to #709's own text" paragraph in the commit message/PR body before merge, since it's a factual claim the build disproves and it will otherwise sit in permanent git history misdescribing which files still need the same fix.
) - `TraceFlow._foutio`'s `#:` comment named a bare `Type[Dumper]`, which Napoleon resolves against every class named `Type` in the project (five of them), so Sphinx warns `more than one target found for cross-reference 'Type'` as soon as anything documents that attribute. - `TraceFlow.__output__` carried the identical bare `Type[Dumper]` in its own `#:` comment, the same latent defect even though nothing currently triggers it. - Qualified both as `~typing.Type[Dumper]`, matching the convention already used for this exact ambiguity in `pcapkit/protocols/__init__.py` and seven sibling files (`frame.py`, `sctp.py`, `link.py`, `tcp.py`, `internet.py`, `udp.py`, `pcapng.py`). - Verified by temporarily mirroring the `autoattribute:: _foutio` directive a pending docs change adds: the bare form reproduces the warning, the qualified form clears it, confirmed against the rendered `typing.Type` link in the built HTML. Build: `sphinx-build -b html` succeeds with no new warnings of any kind.
5e25718 to
f9edcd3
Compare
|
✅ GOOD TO MERGE — the standing ❌ NEEDS CHANGES from |
Detailed re-reviewRe-reviewed on Opus, read-only in a git worktree, against I did not rebuild the docs. Per the re-review brief I am relying on the earlier round's measurement — 53 warnings pristine → 54 with the bare form under an 1. Is the corrected body accurate, and is the retraction plain? — YesThe false claim is gone. Searched the current body for every form of it: no "overstated", no "only two source sites", no "no other file needs changing". What replaced it is an H2 section, I checked each factual assertion the new body makes, not just the retracted one:
No remaining false claim. The body is now more accurate than the average PR body in this repo. 2. Do the four
|
| # | Question | Verdict |
|---|---|---|
| 1 | Body accurate; false claim gone; retraction plain? | Yes — no trace of the "only two sites" claim; retraction is an H2 that names itself wrong; all 8 other factual claims independently verified |
| 2 | Four .rst line numbers still hold post-#694? |
Yes, all four, unshifted — #694's edits to engine.rst/traceflow.rst all landed below line 40, reassembly.rst untouched |
| 3 | Part of #709 correct? Reference the follow-up? |
Part of is correct. Follow-up branch fix/709-rst-type-xrefs is unpushed with no PR, so nothing to reference yet. Stated reason for deferring is stale (#694 merged) — non-blocking |
| 4 | Code unchanged at f9edcd3f3? |
Confirmed — 1 file, +2/−2, lines 146 and 406, exactly as previously verified. Relying on the prior warning-count measurement, not repeating it |
| 5 | typing.Type vs intersphinx (dictdumper)? |
Settled — cannot collide. The warning is emitted only from the local domain inventory; intersphinx matches by exact key and never reports ambiguity |
| 6 | 8-file convention claim | Confirmed; "every one writing ModuleDescriptor[…] | ~typing.Type[…]" is loose for protocols/__init__.py:72. Cosmetic |
The prose defect that caused the block has been retracted, and the retraction is itself correct — I verified every site it cites rather than taking its word. The code was never in question and has not moved. Standing NEEDS CHANGES cleared; good to merge on review grounds, leaving CI as the remaining gate.
…#714) 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.
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 |
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 |
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 |
Part of #709.
TraceFlowBasecarried two#:autodoc comments naming a bareType[Dumper]. Napoleon resolves a bareTypeagainst every class of that name in the project, andpcapkithas several — includingpcapkit/const/l2tp/type.py:18'sclass Type(IntEnum).pcapkit/foundation/traceflow/traceflow.py:146DefaultDict[str, tuple[ModuleDescriptor[Dumper] | Type[Dumper], str | None]]:… | ~typing.Type[Dumper], …pcapkit/foundation/traceflow/traceflow.py:406#: Type[Dumper]: Dumper class.#: ~typing.Type[Dumper]: Dumper class.:406is_foutio, the attribute PR #694 adds anautoattributedirective for — which is what makes thewarning appear rather than stay latent.
:146is__output__, carrying the identical defect with nothingcurrently triggering it.
The fix does more than emit-a-warning-less
Established by a cross-review that built the docs either side of the change: before the fix, Sphinx does not
merely warn — it silently resolves
Typeto the wrong class, linking topcapkit.const.l2tp.type.Type, anL2TP enum, instead of dropping the link. After the fix it resolves correctly, to
typing.Typeexternally andpcapkit.dumpkit.common.Dumperinternally. So this is a wrong-hyperlink fix, not just warning hygiene.Measured with the
autoattribute:: _foutioblock from #694 temporarily added to make the warning reproducible:53 warnings on pristine
main→ 54 with the directive and the bare form → back to 53 with the qualifiedform, byte-identical to pristine.
Why
~typing.TypeIt is the convention the codebase already uses for this exact ambiguity, in eight files, every one writing
ModuleDescriptor[Protocol] | ~typing.Type[Protocol]:After this change, zero bare
Type[references remain in any#:comment underpcapkit/:Correction: an earlier version of this description was wrong, and #709 was right
This description previously claimed that #709 "overstated the spread" and that the four pre-existing
more than one target found for cross-reference 'Type'warnings were these two sites rendered at the base classplus its subclasses. That was wrong. The cross-review built the docs on pristine
mainand traced all fourwarnings to hand-written
:type:fields in three other files, none of them generated fromtraceflow.py:Confirmed independently by grep. Those four warnings still fire unchanged after this merges, so #709's
original claim — that other bare-
Typesites need the same treatment — was correct, and the earlier "there areonly two source sites" was a bad inference from grepping
pcapkit/**/*.pywhen the warnings originate indocs/source/**/*.rst.This PR therefore does not close #709; it is
Part ofit. The four.rstsites live underdocs/, whichopen PR #694 owns, so fixing them here would collide. They are left for a follow-up once #694 lands.
Worth noting for that follow-up:
docs/source/pcapkit/foundation/traceflow/traceflow.rst:30already uses afully qualified form,
typing.Type[pcapkit.protocols.protocol.ProtocolBase], so the.rsttree has its ownconvention distinct from
pcapkit/'s~typing.Type. Whichever is chosen should be applied consistently.No test
This changes documentation text inside a comment, with no runtime behaviour to assert on; the check is the
Sphinx warning diff, stated above. The cross-review noted this matches an explicit pre-existing project policy
documented in
tests/project/test_documentation_claims.py's own docstring.