Skip to content

docs(protocols): second-round pass over the protocol pages (#719) - #1086

Merged
JarryShaw merged 1 commit into
mainfrom
docs/719r2-protocols
Oct 6, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
docs/719r2-protocols

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • docs — documentation only

Description of your pull request and other information

Second-round #719 read of all 46 pages under docs/source/pcapkit/protocols/; 13 changed, 33 left as they were. Every fix is a claim the code contradicts:

  • "contains X only" was false on ftp, rarp, arp, ngap, esp and protocol (checked against each module's __all__). protocol.rst also called Protocol the base of every family, but the built-in families subclass ProtocolBase.
  • ipx: the field is ipx.chksum. mh: removed the todo saying CGA Parameters (type 12) still used the generic handler, since MH.__option__ routes it to cga_param.
  • index: NGAP was missing from the class-hierarchy graph. application/index: the APPTYPE alias target was unresolved; it now points to pcapkit.const.reg.apptype.apptype.AppType.
  • misc/index now names PCAPNG, and c_tag/s_tag now include id() in the identity members they add.

sphinx -b dummy -n on fresh output directories, counting warnings on these pages only: main 1, branch 0, none new.

@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 6, 2026
@JarryShaw JarryShaw added docs Pull requests that change documentation only (docs: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 8e796370b: NEEDS CHANGES (ran on Sonnet; author Opus). There is one wording fix to make.

protocol.rst now says Protocol is the base class for user-defined protocols. That is half true. ext.rst:112-116 tells authors to subclass Link/Internet/Transport/Application, and to inherit Protocol directly only for a new protocol stack. Please say that instead.

Everything else checks out:

  • Every name the pages now list matches its module's __all__: ftp, rarp, arp, ngap, and all 10 in esp.
  • Probed: Protocol.__subclasses__() is [], HTTP resolves through Application to ProtocolBase, and type(Protocol) is ProtocolMeta.
  • ipx uses chksum.
  • The mh CGA_Parameters handler exists (mh.py:1217, :3288).
  • NGAP subclasses Application.
  • The APPTYPE target resolves, and C_Tag/S_Tag define id().
  • No directive target changed.
  • tests/project passes 379.

UNVERIFIED: no Sphinx build.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
- ftp, rarp, arp, ngap, esp, protocol: drop "contains X only" where the
  module defines more (FTP_DATA, DRARP, InARP, the NGAP enums, the ESP
  SA/algorithm types, ProtocolBase/ProtocolMeta); protocol.rst now says to
  inherit Protocol directly only for a new protocol stack, a layer base
  otherwise, and that the built-in families derive from ProtocolBase
- ipx: the checksum field is ipx.chksum, not ipx.cksum
- mh: drop the stale todo; the CGA Parameters option (type 12) is
  registered to _read_opt_cga_param/_make_opt_cga_param
- index: add NGAP to the class-hierarchy graph and group the click
  targets of the application-layer classes together
- application/index: fix the APPTYPE alias target to
  pcapkit.const.reg.apptype.apptype.AppType (was unresolved)
- misc/index: name PCAPNG alongside the PCAP header classes
- c_tag, s_tag: list id() among the identity members each tag adds

sphinx -b dummy -n: 1 -> 0 warnings on these pages, none new;
tests/project passes.
@JarryShaw
JarryShaw force-pushed the docs/719r2-protocols branch from 8e79637 to a34eea9 Compare October 6, 2026 17:38
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on a34eea95b: GOOD TO GO (ran on Sonnet; author Opus, round 2)

The delta from 8e796370b touches only one paragraph in protocol.rst.

  • "Inherit Protocol directly only for a new stack; a layer protocol subclasses Link/Internet/Transport/Application" matches ext.rst:98-114.
  • The built-ins, including those four layer classes, are class X(ProtocolBase[...]), and type(ProtocolBase) is ProtocolMeta, as probed in round one.
  • All four new :class: targets have autoclass entries, and :doc: /ext resolves.
  • The round-one findings stand.

Not re-run for this prose-only delta: tests and Sphinx. The worker reports Sphinx -n as unchanged and tests/project at 379 passed.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw
JarryShaw merged commit ce06a01 into main Oct 6, 2026
26 checks passed
@JarryShaw
JarryShaw deleted the docs/719r2-protocols branch October 6, 2026 17:49
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label 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