Skip to content

docs(foundation): correct the registrar contracts and cut timed context - #1015

Merged
JarryShaw merged 1 commit into
mainfrom
docs/719-foundation-docstrings
Oct 5, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
docs/719-foundation-docstrings

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — N/A, no code changed; proved by AST comparison
  • make test passes, and a test case covers the change — I ran tests/project (268 passed, 1 skipped, 864 subtests) and tests/foundation tests/interface, not the full suite
  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — N/A, docstrings only, no behaviour change

What is the purpose of your pull request?

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • chore — anything else

Description of your pull request and other information

The pcapkit/foundation/** slice of #719 — 21 of 32 files, +186/−170. It adds lines on balance, because the dominant finding was undocumented contract rather than verbose prose.

No code changed, and that is checked rather than asserted. For all 21 files, the AST with every docstring stripped is byte-identical to main (ast.dump equality). The three registrar issubclass checks sit at extraction.py:452, :491, :530 and are untouched.

The registrar contracts were documented one class too narrow. register_engine, register_reassembly and register_traceflow each said the argument must be an Engine/Reassembly/TraceFlow subclass; the runtime checks are against EngineBase/ReassemblyBase/TraceFlowBase, and the shipped classes derive from the base directly. Verified at runtime: all three public classes are strict subclasses of their base, so the documented type was strictly narrower than what is accepted. registry/foundation.py's engine wrapper had the same error, and register_protocol plus nine argument descriptions said Protocol where the check is ProtocolBase. This is the same conflation #513 hit from the caller's side.

All four register_* methods documented neither their failure nor their warning. Each raises RegistryError on a bad class (:407, :453, :492, :531) and emits RegistryWarning on an overwrite (:411, :456, :495, :534). Raises: and Warns: sections are added for all four — that accounts for most of the +186.

One docstring was simply false. Deferred said TCP reassembly builds packet eagerly and that deferring it was left for its own change. reassembly/tcp.py:546 already passes packet=Deferred(...).

Citations are frozen per the ruling on #719: 41 citation occurrences across the subtree — 25 :issue: roles and 16 bare #nnn — with per-file multisets byte-identical. Timed context is cut: "currently" ×6, "today" ×4, "at the time of writing", "yet", and the "used to"/"no longer"/"previously" history in Completion, the conflict docs, the reassembly comments and the trace_format notes. The no_eof behaviour-change note is kept, measurement and citation included, as version-bounded prose.

A code defect found and deliberately not fixed here, because this is a prose PR: the three error messages still read "must be an Engine / Reassembly / TraceFlow subclass" while the checks beside them are against the *Base classes, so the message misnames the accepted type. The register_extractor_engine type hints are also Type[Engine], narrower than the runtime check. Filed separately rather than smuggled in.

Not done: concision proper. The long extraction.py docstrings (no_eof, _owns_input) and the register_apptype argument prose are untightened, and 11 of the 32 files are untouched. No Sphinx build; cross-reference targets were checked by importing them instead. One tests/foundation test fails on a missing generated capture (test.pcap) in a fresh worktree, which is the known fixture gap rather than anything in this diff.

@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 4, 2026
@JarryShaw
JarryShaw force-pushed the docs/719-foundation-docstrings branch from 379ed8e to 35a78a3 Compare October 4, 2026 23:02
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES at 379ed8ea5, fixed at 35a78a371 — opus cross-review, a different model from the sonnet that drafted the slice. Its AST check matched mine: all 21 files identical to main with docstrings stripped, and it went further by confirming no non-docstring string literal changed either.

It found the job half-done, which is fair. The slice corrected register_extractor_engine but left its two siblings in the same file still naming Reassembly/TraceFlow, and the two extraction.py siblings still led with the public class while only parenthetically admitting the base. One contract documented three ways, in a PR whose purpose is that contract. All four now use one phrasing, leading with the base.

Measured while fixing it, and it strengthens the case: no built-in class subclasses the public class at all — 8 engines, 3 reassembly classes, 1 trace-flow class, every one deriving from the base directly. The old prose named a type nothing shipped satisfies.

Four cross-references pointed at an attribute the named class does not own. They cited Reassembly.__callback_fn__ / TraceFlow.__callback_fn__. It is in Reassembly.__dict__ but not in ReassemblyBase, and IPv4, IPv6 and both TCP classes each own their own copy with ReassemblyBase/TraceFlowBase in the MRO rather than the public class — so cls.__callback_fn__ is Reassembly.__callback_fn__ is false for all four. The docs named a different object from the one the registrar writes to. Repointed to the concrete classes, which is what the adjacent # NOTE: comments already said.

Both optional items taken: the Type[Engine] hint being narrower than the check is now noted in prose (the hint itself stays for #1016), and the four Raises: sections now say "not a class, or not a … subclass", since a non-class argument raises TypeError from issubclass before RegistryError is reached.

Re-verified at the new head: AST identical to main across all 21 files, citations frozen at 41 occurrences with per-file multisets unchanged. The one tests/foundation failure is the known missing-capture gap, not this diff. Delta re-review dispatched.

- `Extractor.register_engine`, `register_reassembly` and `register_traceflow` said
  the argument must be an `Engine`, `Reassembly` or `TraceFlow` subclass. The
  checks are `issubclass(..., EngineBase / ReassemblyBase / TraceFlowBase)`
  (`extraction.py:452`, `:491`, `:530`), and the shipped classes derive from the
  base directly, so the prose understated what is accepted. The wrapper in
  `registry/foundation.py` carried the same error.
- `register_protocol` and nine argument descriptions said `Protocol` where the
  check is `issubclass(protocol, ProtocolBase)`.
- All four `Extractor.register_*` methods raise `RegistryError` on a bad class and
  emit `RegistryWarning` on an overwrite, and documented neither. Added `Raises:`
  and `Warns:` sections, which is why this slice adds lines rather than cutting.
- `Deferred`'s docstring said TCP reassembly builds `packet` eagerly and that
  deferring it was left for its own change. `reassembly/tcp.py` already passes
  `packet=Deferred(...)`, so the claim was false.
- Cuts timed context per #719 — "currently", "today", "at the time of writing",
  "yet", and the "used to"/"no longer"/"previously" history in `Completion`, the
  `conflict` docs, the reassembly comments and the `trace_format` notes. The
  `no_eof` behaviour-change note and its measurement are kept as a
  version-bounded note, citation included.

- Brings all four registrar docstrings to one phrasing. Two wrappers in
  `registry/foundation.py` still named `Reassembly`/`TraceFlow`, and the two
  `extraction.py` siblings still led with the public class, so one contract read
  three ways. Measured: **no** built-in class subclasses the public class -- 8
  engines, 3 reassembly, 1 trace flow all derive from the base directly.
- Repoints four `__callback_fn__` cross-references from `Reassembly`/`TraceFlow`
  to the concrete `IPv4`/`IPv6`/`TCP` classes. The attribute is in
  `Reassembly.__dict__` but not in `ReassemblyBase`, and each concrete class owns
  its own copy, so the docs named a different object from the one written to.
- Notes that the `Type[Engine]` hint is narrower than the check, and that a
  non-class argument raises `TypeError` before `RegistryError` is reached.

- Adds the four missing `.. autoattribute:: __callback_fn__` entries under
  `docs/source/pcapkit/foundation/`. Repointing the cross-references above left
  them dangling: the attribute was autodoc'd only on `Reassembly` and
  `TraceFlow`, and `autodoc_default_options` sets no `inherited-members`, so the
  new targets had no inventory entry and rendered as plain text. `nitpicky` is
  unset, so the build would not have complained. Verified by a Sphinx build:
  all four anchors now exist and `registry.html` links to them.

No code changed: for all 21 files the AST with docstrings stripped is identical to
`main`. Citations are frozen — 41 per-file multisets, byte-identical.

`tests/project`: 268 passed, 1 skipped, 864 subtests passed. Part of #719.
@JarryShaw
JarryShaw force-pushed the docs/719-foundation-docstrings branch from 35a78a3 to 42cc77f Compare October 4, 2026 23:14
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES at 35a78a371, fixed at 42cc77f75 — opus delta re-review. It confirmed all five of my fixes landed, then caught that fix 3 had created a new defect: the four repointed :attr: targets did not resolve.

__callback_fn__ is autodoc'd in exactly two places in all of docs/source/ — on Reassembly and TraceFlow, which are precisely the two targets the repointing removed. The four concrete classes document only __protocol_name__ and __protocol_type__, and autodoc_default_options sets no inherited-members with private-members commented out, so the new targets had no inventory entry. Four dead links rendering as plain text — and since nitpicky is unset, the build would not have complained.

Fixed by documenting rather than reverting: a .. autoattribute:: __callback_fn__ with :no-value: added to reassembly/ip/ipv4.rst, reassembly/ip/ipv6.rst, reassembly/tcp.rst and traceflow/tcp.rst, matching the existing Reassembly/TraceFlow entries. Verified by an actual Sphinx build whose log proves it read the worktree: all four anchors exist and registry.html links to them. The bases are deliberately not given the attribute — ReassemblyBase and TraceFlowBase do not own it, as cls.__callback_fn__ = [] runs per subclass in reassembly.py:498 and traceflow.py:451.

The reviewer corrected its own earlier count, in the right direction. It had said 6 engines and 7 reassembly/trace-flow classes from hand-listing module names; enumerating via pkgutil gives 8 concrete engines — it had missed pcap.PCAP and pcapng.PCAPNG. My 8/3/1 figures stand, and issubclass(..., Engine) is False for all twelve classes.

It also advised keeping the repeated parenthetical in all four registrar docstrings rather than shortening it, on the grounds that the surprising half — that no built-in satisfies the public class — is exactly what the old text got wrong, and that a caller reaches these four independently rather than reading them in sequence. Taken.

And it measured what I had only described: all four registrars raise TypeError: issubclass() arg 1 must be a class on a non-class argument, before RegistryError is reached, so "not a class, or not a … subclass" is accurate.

@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 42cc77f75 — opus, third pass. It ran both Sphinx builds itself rather than taking mine, and closed the one gap I had flagged.

The warning count is provably unchanged. Base and head both give build succeeded, 61 warnings, and diffing the normalised warning multisets leaves exactly one difference — a pre-existing _AT cross-reference warning in reassembly/tcp.rst moving from line 371 to 373, which is precisely the two lines the autoattribute insertion added. Zero new warnings, zero removed, and none mentions callback_fn.

The fix is build-proven, not argued. All four anchors exist in the generated HTML, and registry.html emits an href to each. The before/after is sharp: at base, registry.html carried 2 distinct __callback_fn__ hrefs — both on Reassembly/TraceFlow — and a grep for the IPv4 anchor returned ABSENT. At head it carries 4, all on the concrete classes, each with a matching id.

It also checked tree provenance rather than assuming it: each build log carries 11 references to its own worktree and exactly 3 to the shared checkout, all three being .venv/.../site-packages internals identical on both sides. Neither build documented the wrong tree.

Leaving the bases undocumented is more clearly right than I put it. hasattr is False on both ReassemblyBase and TraceFlowBase, and attribute access raises AttributeError — so documenting it there would document something that does not exist at runtime, not merely something they do not own. The six owners hold 6 distinct list objects out of 6, so each entry documents a genuinely separate registry, and the documented surface now matches the runtime surface exactly: six anchors, none on either base.

One honest limitation it recorded: it did not build the intermediate head 35a78a371, so the claim that that sha's four targets were dead stays statically derived — corroborated by the base build's ABSENT anchor, but not proven by a build of that exact commit. It does not affect this verdict and I would rather have the distinction on the record.

Everything from the earlier rounds carries unchanged against the same unmoved base: the three issubclass guards, register_protocol's nine argument lines, the eight Raises:/Warns: line numbers, Deferred, the no_eof note, the Formats keys, the four TypeError measurements, and the 8/3/1 built-in counts.

@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 4, 2026
@JarryShaw
JarryShaw merged commit 2c7da9e into main Oct 5, 2026
73 checks passed
@JarryShaw
JarryShaw deleted the docs/719-foundation-docstrings branch October 5, 2026 00:36
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 5, 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