Repository navigation
docs: prefer Protocol over ProtocolBase in cross-references, per the owner's #1103 ruling - #2420
Merged
Merged
Conversation
JarryShaw
added
the
docs
JarryShaw
added
the
review: running
Contributor
|
Coverage: 91.73% (unit tier, Python 3.15,
Per-file detail: the |
Owner
Author
|
Review: GOOD TO GO at Non-blocking: the body understates the remaining backlog (54 non-test |
JarryShaw
added
review: good-to-go
Owner
Author
|
Use the last documented override. I see that we replaced the reference in the Internet to Protocol. That's not accepted. |
JarryShaw
added
review: needs-changes
Owner
Author
|
Got it — confirmed |
) ProtocolBase is the internal hidden API and Protocol is the one exposing the attributes, so a plain "here is where this member is documented" link targets Protocol. The owner then refined this: "Use the last documented override." A docstring describing what a *specific* class overrides has to name the real ancestor in that class's own chain, which is never Protocol -- Protocol has no subclasses at all, so ProtocolBase is the last documented override for every class in the tree. * Plain documentation links go to Protocol.<member>: the module-level prose and `#:` comments in protocol.py, register_protocol/_register_name in registry/protocols.py, the `register` Arguments and versionchanged clauses in internet.py, link.py, transport.py and sctp.py, and the unrelated-class references in sentinels.py, data/data.py, misc/pcap/{frame,header}.py, ipv6.py, utilities/config.py, conf.py and the mptcp test. * Override-describing links go to ProtocolBase.<member>: Internet.{register, from_data,_make_payload}, Link.register, Transport.register, IPv6_Ext.protocol, the ipv6_ext module docstring and ipv6_ext.rst, the {AH,ESP,HOPOPT,MH}.alias MRO sentences, NoPayload.protochain, PCAPNG.{length,packet}, HTTP.from_data and HTTPv2's HTTP.unpack. Each of these says "As X", "the base", "the inherited", "narrows", "sits between ... in the MRO" or "the guard this shares with", and the surrounding sentences already name ProtocolBase. * The `Raises:` clause of each `register` keeps naming ProtocolBase, unchanged: #2316/#1016 ruled that it names the class the guard's `issubclass` actually tests while the raised message names Protocol, and tests/protocols/test_register_class_guard_unit.py asserts exactly that. * Class-attribute declaration pointers stay on Protocol -- Protocol.__data__ (declared on Protocol itself), Protocol.__keywords__ and Protocol.__own_make_keywords__ -- since they name where the knob is documented rather than a method a class overrides. * ProtocolBase.__init_subclass__ references are still left alone (l2tpv2.py, c_tag.py, protocol-layer-placement.rst): Protocol does override __init_subclass__, so those name the base implementation deliberately. Verified Protocol in no class's __mro__ but its own, and that ProtocolBase defines and documents every member involved while Protocol declares only __init_subclass__ and __data__. AST-structure guard (ast.dump with docstrings stripped) confirms all 12 touched .py files are prose-only against 27c797d; ipv6_ext.rst carries no code. The mptcp test, test_docstring_contract.py, test_conventions_doc_claims.py, test_register_class_guard_unit.py and the docs xref suites pass (91 passed, 22 skipped, 444 subtests), as do tests/protocols, tests/foundation, tests/corekit and tests/utilities in full (3414 passed, 18 skipped, 60258 subtests).
JarryShaw
force-pushed
the
fix/1103-protocol-not-protocolbase
branch
from
October 28, 2026 21:03
6c972b8 to
891f85f
Compare
JarryShaw
added
review: running
Owner
Author
|
Review: GOOD TO GO at Three sites flagged as non-blocking ( |
JarryShaw
added
review: good-to-go
Owner
Author
|
Updating the 21 reverted sites to the split display/target form: |
JarryShaw
added
review: needs-changes
JarryShaw
added a commit
that referenced
this pull request
Oct 28, 2026
The owner refined the #1103 ruling: a `ProtocolBase.<member>` reference should read `Protocol.<member>` in the rendered docs, for one consistent public vocabulary, while the link still targets the `ProtocolBase` attribute. That is the split-display-target form, `:meth:`Protocol.X <...protocol.ProtocolBase.X>``, applied across everything Sphinx renders. * 46 reference sites in `pcapkit/` and `docs/source/` now display `Protocol.<member>`. 37 keep their `ProtocolBase` target, which is the requested form; the other 9 already targeted `Protocol` -- #2420 moved those targets but left the display text reading `ProtocolBase` -- and are now display-consistent too. * Three short-form references in `protocol.py` gained an explicit fully-qualified target, so the display text could change without repointing the link: `:meth:`ProtocolBase._parse_next_layer`` is now `:meth:`Protocol._parse_next_layer <...ProtocolBase._parse_next_layer>``. * `__init_subclass__` keeps displaying `ProtocolBase` (6 sites). `Protocol` defines its own override, so displaying `Protocol` there would name a different method than the link resolves to. * Two further sites keep `ProtocolBase` because the adjacent prose would otherwise contradict itself: `schema.py`'s comment reads "``ProtocolBase``, not ``Protocol``" two lines above its reference, and `protocol-layer-placement.rst` asserts `OSPF.__proto__` *is* that attribute inside a paragraph analysing MRO set-difference. * The `Raises:` clause of `register` is unchanged. It names the class the guard's `issubclass` actually tests, not a member whose documentation is being pointed at, and `tests/protocols/test_register_class_guard_unit.py` asserts that wording (#2316/#1016). * `tests/` and `examples/` are untouched (28 sites). Their docstrings are not part of the Sphinx render, so the display rationale does not reach them. No link was repointed: a guard canonicalises every role target before and after and reports them identical, bar the three short-form expansions, which resolve to the same object. An AST-structure guard (`ast.dump` with docstrings stripped) confirms all 26 touched `.py` files are prose-only.
5 tasks done
JarryShaw
added a commit
that referenced
this pull request
Oct 29, 2026
Applying the #1103 ruling to the member references #2420 left behind: a `ProtocolBase.<member>` cross-reference now reads `Protocol.<member>`, for one consistent public vocabulary, and links there too. Both halves move, because on the rendered page that is where the documentation is. `protocol.rst` documents the members with nested `autoproperty`/`automethod` directives under `.. autoclass:: ...Protocol`, so `Protocol.<member>` carries the signature and the docstring; the `ProtocolBase` block is `:no-members:` plus grouped `:no-index-entry:` stubs whose body only links back to the `Protocol.*` entries, and the general index lists `Protocol.*` alone. Measured on the pinned docs toolchain (`Sphinx==9.2.0`). * 46 sites in `pcapkit/` and `docs/source/`. 37 move display *and* target to `Protocol.<member>`; the other 9 already targeted `Protocol` and only needed the display text. The explicit target is the fully-qualified spelling the house style asks for -- the form 124 existing references already use. * `from_data` is consistent again: `internet.py` and `application.py` pointed at `ProtocolBase.from_data` where `link.py` and `transport.py` pointed at `Protocol.from_data` for the same kind of reference. All four now read alike. * `__init_subclass__` keeps `ProtocolBase` (6 sites), since `Protocol` overrides it and naming `Protocol` would name a different method. So do `schema.py` and `protocol-layer-placement.rst`, whose adjacent prose would otherwise contradict itself, and `register`'s `Raises:` clause, which names the class the guard's `issubclass` tests (#2316/#1016). * `changelog/1.5.0.rst` keeps its five `ProtocolBase.<member>` references. Each names the class a *built-in* protocol inherits the member from -- the built-ins derive from `ProtocolBase`, not `Protocol` -- and `CHANGELOG.md` is generated from that file by `util/changelog_md.py`, where a role flattens to a code span with no link, so moving them would rewrite release-note prose rather than an href. * `tests/` and `examples/` are untouched (28 sites): their docstrings are not part of the Sphinx render. Hrefs only. A role-canonicalisation guard reports no role and no display-text change beyond the above, and an AST-structure guard (`ast.dump` with docstrings stripped) is identical for every touched `.py` file.
JarryShaw
added a commit
that referenced
this pull request
Oct 29, 2026
display rule (#2422) Applying the #1103 ruling to the member references #2420 left behind: a `ProtocolBase.<member>` cross-reference now reads `Protocol.<member>`, for one consistent public vocabulary, and links there too. Both halves move, because on the rendered page that is where the documentation is. `protocol.rst` documents the members with nested `autoproperty`/`automethod` directives under `.. autoclass:: ...Protocol`, so `Protocol.<member>` carries the signature and the docstring; the `ProtocolBase` block is `:no-members:` plus grouped `:no-index-entry:` stubs whose body only links back to the `Protocol.*` entries, and the general index lists `Protocol.*` alone. Measured on the pinned docs toolchain (`Sphinx==9.2.0`). * 46 sites in `pcapkit/` and `docs/source/`. 37 move display *and* target to `Protocol.<member>`; the other 9 already targeted `Protocol` and only needed the display text. The explicit target is the fully-qualified spelling the house style asks for -- the form 124 existing references already use. * `from_data` is consistent again: `internet.py` and `application.py` pointed at `ProtocolBase.from_data` where `link.py` and `transport.py` pointed at `Protocol.from_data` for the same kind of reference. All four now read alike. * `__init_subclass__` keeps `ProtocolBase` (6 sites), since `Protocol` overrides it and naming `Protocol` would name a different method. So do `schema.py` and `protocol-layer-placement.rst`, whose adjacent prose would otherwise contradict itself, and `register`'s `Raises:` clause, which names the class the guard's `issubclass` tests (#2316/#1016). * `changelog/1.5.0.rst` keeps its five `ProtocolBase.<member>` references. Each names the class a *built-in* protocol inherits the member from -- the built-ins derive from `ProtocolBase`, not `Protocol` -- and `CHANGELOG.md` is generated from that file by `util/changelog_md.py`, where a role flattens to a code span with no link, so moving them would rewrite release-note prose rather than an href. * `tests/` and `examples/` are untouched (28 sites): their docstrings are not part of the Sphinx render. Hrefs only. A role-canonicalisation guard reports no role and no display-text change beyond the above, and an AST-structure guard (`ast.dump` with docstrings stripped) is identical for every touched `.py` file.
3 of 5 tasks
JarryShaw
added a commit
that referenced
this pull request
Oct 29, 2026
…ce sweep, fix other prose drift (#2428) Round 8 of the whole-repo prose sweep (#1103). Fanned out 8 shard agents by directory; most of the repo was clean. Changes: * pcapkit/protocols/internet/ipv6_ext.py + its .rst: finish the carry-over miss from round 7 -- the `_lookup_next_layer` plain pointer still targeted `ProtocolBase`, flipped to `Protocol` per #2420's ruling. * Same miss in corekit/sentinels.py (`__keywords__`), docs/source/ext.rst (three sites, including a verbatim twin of a paragraph #2420 already fixed in protocol.py), schema/schema.py and schema/link/ethernet.py. * internet.py, link.py, transport.py, sctp.py, protocol.py: `register()`'s Arguments: and a couple of Returns:/versionchanged clauses said `Protocol` subclass/instance while the signature types `ProtocolBase` and the adjacent Raises: clause already said so -- internal contradiction, fixed to `ProtocolBase` to match. * docs/source/contributing/conventions/protocol-layer-placement.rst: the Link-Application-ProtocolBase set difference is `__proto_key__`, not empty as claimed (#2343 added it after this page was written). * docs/source/contributing/conventions/registry-protocol.rst: "every registry overrides `_validate_value`" contradicted its own next paragraph, where the AppType transports inherit rather than override. * pcapkit/vendor/ipx/packet.py, vendor/reg/linktype.py: a self-contradicting claim and a stale counted-number, both in vendor docstrings. * docs/source/engines.rst, docs/source/conf.py, util/changelog_md.py, CONTRIBUTING.md: a stale scapy version, a wrong file path, a false "matches the Sphinx we build with" claim, and a missing caveat about actionlint making Lint non-advisory. No executable code changed -- every edit is inside a docstring, comment, or .rst file; confirmed by diff review and `import pcapkit` from this worktree. Four behavioral defects found but deliberately left unfixed (reported separately): 16 of 24 pcapkit/vendor crawlers' `process(self, data)` reads the module-global `DATA` instead of its own `data` argument (latent, harmless only because `request()` returns that same global); an empty `FilterType` registry behind a live TODO; three open "handle this programmatically" TODOs in pcapkit/vendor/mh; and dead `multiprocessing.Pool` scaffolding in vendor/__main__.py.
Open
4 of 5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please follow the guide below
You will be asked some questions, please read them carefully and answer honestly
Put an
xinto all the boxes [ ] relevant to your pull request (like that [x])Use Preview tab to see how your pull request will actually look like
Searched for similar pull requests
Followed the coding style (
make pylint,make mypy,make isort)make testpasses, and a test case covers the changeAdded a changelog entry under
docs/source/changelog/and regeneratedCHANGELOG.md, if the change is user-visible — N/A, prose only and not user-visible, as for round 1 (docs: #719-style prose sweep after the #1090-1102 defect wave #2408) and round 2 (docs: #1103 round 2 whole-repo prose sweep #2415)What is the purpose of your pull request?
Tick the commit type your subject line carries.
fix— corrects a defectfeat— adds a featureperf— changes performance, not behaviourrefactor— changes neither behaviour nor performancetest— tests onlydocs— documentation onlyci— workflows or build toolingrelease— bumps the version or rolls up a distributionchore— anything elseDescription of your pull request and other information
The owner ruled on #1103 the opposite of what both earlier rounds assumed: "
ProtocolBaseis the internal hidden API.Protocolis the one exposing the APIs and attributes. So I'd prefer we keep referring onProtocolwhenever possible." Round 1 (#2408) already merged with the wrong-direction conversion in theregister/register_protocoldocs; round 2 (#2415) was closed unmerged for the same reason.This PR reverts #2408's merged changes and finishes the direction correctly:
pcapkit/protocols/protocol.py,internet/internet.py,link/link.py,transport/transport.py,transport/sctp.py,foundation/registry/protocols.py— theregister/register_protocol/register_protocol_codedocs go back toProtocol.<member>, including fixing pre-existing display/target mismatches (ProtocolBase.register <...Protocol.register>-style) the same way.register's ownRaises:clause still namesProtocolBase. docs(protocols): register's Raises name Protocol in some layers, ProtocolBase in others #2316/fix(foundation): registrar error messages and type hints name the public class, not the Base the guard checks #1016 already ruled on this split — the clause names the class the guard'sissubclasscheck actually tests, while the raised message namesProtocol.tests/protocols/test_register_class_guard_unit.pyasserts this and caught a first pass of this PR that flipped it by mistake.protocol.py,misc/null.py,application/httpv2.py, the mptcp test andipv6_ext.rst(one,schema/internet/ipv6_route.py, was already correct — no change needed there).pcapkit/anddocs/for otherProtocolBase.<member>cross-reference targets where the member is plainly inherited (not somethingProtocolBaseuniquely has):__init__,__post_init__,_make_data,from_data,alias,length,packet,protocol,unpack,__keywords__,__own_make_keywords__,__data__,_lookup_next_layer,_import_next_layer,_read_packet,expand_comp— acrosssentinels.py,data/data.py,misc/pcapng.py,misc/pcap/{frame,header}.py,internet/{hopopt,esp,mh,ipv6,ah,ipv6_ext}.py,application/http.py,utilities/config.pyandconf.py.ProtocolBase.__init_subclass__inlink/l2tpv2.py,link/c_tag.pyandprotocol-layer-placement.rst—Protocoloverrides__init_subclass__, so these name the base implementation's distinct schema/data-resolution behaviour on purpose, not the plain-inherited-member case.protocol-layer-placement.rst's__proto__reference is a structural MRO discussion namingProtocolBaseexplicitly as part of that argument, likewise left alone.Round 3 refinement: the owner further ruled "use the last documented override" — within a class's own docstring describing what it overrides, the reference must point at the real ancestor in that class's MRO, not
Protocol. Confirmed by probing the live hierarchy:Protocol.__subclasses__()is empty — every one of the 44 loaded layer classes (Internet,Link,Transport,HTTP,PCAPNG, etc.) descends fromProtocolBaseonly, never throughProtocol. 21 override-framing sites ("the guard this shares with", "As X, but...", "Narrows... declared type", etc.) were reverted back toProtocolBaseacrossinternet.py,link.py,transport.py,ipv6_ext.py(+rst),ah.py,esp.py,hopopt.py,mh.py,null.py,pcapng.py,http.py,httpv2.py; plain "here's where this is documented" links were left asProtocol. Three more sites (http.py'sProtocol.__init__×2,ipv6_ext.py'sProtocol.__own_make_keywords__) read the same way under a strict reading of the rule but weren't caught this round — flagging for a follow-up rather than blocking.Scope not reached: ~37 test files under
tests/protocols/andtests/project/carry the sameProtocolBase.<member>pattern (pre-existing, untouched by this PR either direction). Grep for the full list:grep -rln ':\(class\|meth\|attr\|func\):\[^`]protocol.ProtocolBase.[A-Za-z_]' --include=".py" tests/`. Left for a dedicated #1103 round, per scope.Test plan
ast.dumpviautil/vendor_code_changed.py'scode_of, with docstrings and bare string literals stripped: all touched.pyfiles (18 across both rounds) are prose-only against27c797d35, 0 differences.tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py,tests/test_docstring_contract.py,tests/project/test_conventions_doc_claims.py,test_register_class_guard_unit.py— 100 passed, 22 skipped, 444 subtests passed.tests/protocols,tests/foundation,tests/corekit,tests/utilities(make test's exclusions) — 3414 passed, 18 skipped, 60258 subtests passed, 0 failures. The first pass caught theRaises:clause mistake above (test_register_class_guard_unit.py, 6 SUBFAILED); fixed and reconfirmed green.