Skip to content

docs(schema): convert bare citations to the issue role in docstrings - #1004

Merged
JarryShaw merged 1 commit into
mainfrom
docs/989-schema-citations
Oct 3, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
docs/989-schema-citations

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

  • docs — documentation only

Description of your pull request and other information

Fifth tranche of #989, after #1000 (corekit), #1002 (foundation and friends) and #1003 (protocols/internet). Covers the rest of pcapkit/protocols/ — schema, transport, application, link, misc and data.

Scope follows the ruling on #989: plain # comments keep the bare form, so only docstrings and #: autodoc doc-comments convert. Other string literals are deliberately excluded — the citation in the SchemaError f-string at pcapkit/protocols/schema/schema.py:338 stays bare, because a role there would print as raw markup in a user-facing traceback.

Tokenised by token kind, base e8a60d153 → head:

base head
bare 3-or-more-digit in strings 77 0
bare 3-or-more-digit in #: doc-comments 20 0
bare 1-or-2-digit in strings 9 9, untouched
bare citations in plain # comments 85 85, untouched
hyperlink forms in plain # comments 1 block untouched
explicit issues/NNN hyperlinks 14 0
:issue: roles 0 111

111 roles = 97 bare sites + 14 explicit hyperlinks, across 48 distinct numbers, every one an issue rather than a pull request. The 9 survivors are RFC packet-diagram labels — Chunk, Gap Ack Block, Address Type and Missing Param Type in transport/sctp.py, and TF type #1/#2 in schema/internet/hip.py.

The diff is provably pure markup: collapsing every role and every hyperlink back to #NNN makes the removed and added text identical across all 25 files. ast.dump with every docstring blanked is also identical, so no behaviour can change. 91 of the 111 roles sit in docstring-position literals and the other 20 in #: comments, which ast does not see. Docs build warning sets are identical before and after, including the two pre-existing docutils errors.

@JarryShaw JarryShaw added docs Pull requests that change documentation only (docs: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 3, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES at 995eacaac — opus cross-review, a different model from the sonnet agent that wrote the diff. One real defect, which I reproduced before accepting it:

pcapkit/protocols/schema/transport/tcp.py:279 nests a role inside strong emphasis. **:issue:591 fixed it** is invalid reStructuredText — inline markup cannot contain inline markup — and it renders as the literal text :issue:`591` in bold with no link at all. Rendered both forms through docutils with a stub role and -b pseudoxml:

  • **:issue:591 fixed it** → <strong>:issue:591 fixed it</strong>
  • :issue:591 **fixed it** → <reference refuri=".../issues/591">#591</reference> then <strong>fixed it</strong>

Sphinx emits no warning for it, which is exactly why a green build and identical warning sets are not evidence here. The second form is being applied. I scanned every string token in the six trees for a role inside **…**, *…* or ``…`` — this is the only such site.

Two corrections to the description, now fixed above:

  • The body said docstrings, other string literals and #: comments convert. The rule actually applied is docstrings and #: only: the citation in the SchemaError f-string at schema/schema.py:338 stays bare, because a role would print as raw markup in a traceback. That is the right call and the body now names it.
  • The plain-comment row counted 85 bare citations but silently omitted the hyperlink form in a plain comment at misc/pcapng.py:1132–1135. Measured: 85 bare at base and head, 0 of them shorter than three digits, and the one hyperlink block untouched. The reviewer's 87 counts that block as two citations; the authoring agent's 84 reproduces neither.

Everything else held, including the whole-diff collapse-back and ast.dump equality. Verdict will be re-issued on the amended head.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 3, 2026
…cstrings (#989)

- Replace bare `#NNN` citations in docstrings and `#:` attribute
  doc-comments with `:issue:` under schema, transport, application, link,
  misc and data; plain `#` comments keep the bare form per the #989 ruling.
- Fold the explicit `#NNN <.../issues/NNN>`__ hyperlinks in the pcapng
  docstrings into the same role.
- Leave the one- and two-digit `#N` packet-diagram labels in sctp.py and
  hip.py untouched; they are RFC diagram text, not citations.

Markup only: ast equivalence with docstrings blanked, docs build warning
set unchanged, targeted protocol tests pass.
@JarryShaw
JarryShaw force-pushed the docs/989-schema-citations branch from 995eaca to e95ecba Compare October 3, 2026 12:36
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 3, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at e95ecbaac — opus delta round on the amended head. The fix holds, and the review strengthened three of my own checks rather than just agreeing with them.

Confirmed with a real Sphinx build using this repo's extlinks map, not just a docutils parse: the old form rendered the literal role text in bold with no link, the new form renders an extlink-issue anchor to /issues/591 followed by bold fixed it, and both built with zero warnings — so the defect really was invisible to CI.

One rendering delta I under-stated earlier. I said the sentence reads identically word-for-word, which is true of the words but not of the emphasis: the base bolded #591 along with "fixed it", and the role form cannot sit inside **…**, so the bold span now covers two tokens instead of four. Unavoidable once the citation is linkified, and worth the trade, but it is a rendering change rather than a pure markup one.

Three checks came back stronger than mine:

  • The nested-markup scan was redone with docutils doing the parsing — 133 files, 4,320 chunks, all 14 roles and 6 directives dummy-registered, flagging any role surviving as raw text inside strong/emphasis/literal/reference/substitution/target/footnote/math. 0 hits at head and 0 at base. That catches spans crossing a line break, which my single-line regexes could not see.
  • Collapse-back was applied to whole file contents rather than diff text, per file, and additionally asserted that every explicit link's label matches its target — it never fired, so no pre-existing label/target mismatch was masked. Exactly one residual delta across 25 files: the intended **#591 fixed → #591 **fixed.
  • The docs-rebuild gap I flagged was closed: full docutils warning tallies with line numbers stripped are identical base and head, 37 keys / 238 messages, diff clean.

Also a correction to a number I relayed: the corekit precedent carries 30 instances of GitHub issue :issue: (38 under a wider issue(s) :issue: pattern), not the 36 I passed on. The consistency argument for leaving that phrasing alone stands either way.

Not ready to merge yet — 7 CheckRun legs still in flight, 0 failures.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 3, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Ready to merge at e95ecbaac — closing the "not yet" on the verdict above. CI is complete: 69 CheckRun legs green, 3 skipped, 0 failures, 0 in flight, mergeStateStatus CLEAN. The opus delta verdict stands at this head; nothing has been pushed since.

Yours to merge.

@JarryShaw
JarryShaw merged commit 326a0e5 into main Oct 3, 2026
73 checks passed
@JarryShaw
JarryShaw deleted the docs/989-schema-citations branch October 3, 2026 13:10
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 3, 2026
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Pull requests that change documentation only (docs: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant