Conversation
JarryShaw
added
the
docs
Owner
Author
Kept private-target directives, with the reference that justifies each590 single-underscore directives stay. For each, one rendered reference that resolves to it is named; many have several.
|
JarryShaw
force-pushed
the
docs/private-directive-sweep-684
branch
from
September 23, 2026 03:24
de6c660 to
33bd693
Compare
JarryShaw
changed the title
5 tasks
Owner
Author
|
NEEDS CHANGES Independent cross-review, different model from whichever produced this diff. Pinned to head I was briefed against the original cross-reference-only rule, then mid-review the owner posted a superseding rule (contract-vs-recipe: document what a user needs to use a class and what an implementer needs to subclass it; hide per-code recipe; most private attributes stay by default; anything abstract or required-to-implement-across-subclasses must be documented). Everything below judges the diff against the new rule, not the old one. The count: 590 is exactly right, not a different-scope approximationReproduced the whole-tree regex count myself: Reconciling 701 → 590 to zero remainder, not just "close":
Verdict on the count: CONFIRMED, exactly, not merely "correct under a stated definition." My colleague's 865/701/164 are reproducible and internally consistent, but that regex is coarser than the PR's own definition of "private member" (it also catches private module segments). Once that's corrected and hip.rst is set aside as explicitly out of scope, the two numbers agree to the last digit. Per-claim verdicts (original brief, still valid — these are about rendering safety, independent of which policy is in force)
The substantive re-review under the new "contract vs recipe" rule — this is why the PR needs changes
|
Owner
Author
Correction: the "590" figure above is wrong, and the rule it was measuring has been supersededTwo things to put right in the comment above. The number"590 single-underscore directives stay" is not reproducible, and I withdraw it. It was the length of my Here is a definition anyone can re-run, and the figures it produces. Count every line matching
So 855 → 691, a delta of 164, which is the number of directives that revision removed. On the reviewer's count of 865 → 701: the delta agrees exactly at 164, so we do not disagree about The ruleMore importantly, the analysis this comment records was measuring "is it cross-referenced from The branch has been rebuilt from |
Owner
Author
The three failing checks on this PR are not caused by it — filed as #702
This PR touches only Verified pre-existing, rather than assumed:
Root cause in one line:
Separately, this PR's own verdict stands at NEEDS CHANGES for unrelated reasons — the directive set is |
JarryShaw
force-pushed
the
docs/private-directive-sweep-684
branch
from
September 23, 2026 04:21
33bd693 to
dd53063
Compare
…enet (#684) The API reference should keep usage and extensibility clear while hiding the recipe. Measured against that, `docs/source/` was wrong in both directions: it documented a handful of module-level privates on nobody's surface, and it omitted 39 class private attributes that are squarely contract -- several of them the only undocumented member of a group whose siblings all have directives. * Removed 7 directives, all module-level privates: `esp._resolve`, `esp._CRYPTO`, `ngap._convert`, `ngap._revert`, `ngap._PYCRATE`, `ngap._PDU_LOCK` and the private stream adapter `pypcapfile._NamedStream`. A private helper, a lazily imported backend flag, an internal lock and a wrapper nobody constructs or subclasses. `_NamedStream`'s two nested members go with it, which is correct -- a member of a private class is reachable only through that class. * Added 39 `autoattribute` directives for class private attributes that carry contract. The clearest are the ones that were alone in being left out: `Extractor._flag_f`, the only one of ten `_flag_*` without a directive; `PCAP_CT._backend` and `PyPCAP._backend`, whose every sibling on the same class is documented; and the built-in `PCAP` and `PCAPNG` engines, which carried none of the private state block that all six third-party engines carry. The rest back a documented property, or are read and written directly by collaborators -- `Extractor._vfunc`, `_fext` and `EngineBase._extractor` are what `ext.rst` already teaches engine authors to use by name. * Documented the const enums' `_missing_` fallback once, on the `pcapkit.const` landing page, rather than on each of the 121 enumerations under `pcapkit/const/` that implement it. Looking a value up by an unassigned-in-range number mints a member instead of raising, which is deliberate per #647 and is the extensibility behaviour of the whole package; `conf.py` already names `_missing_` in `exclude-members`, so per-class directives would be fighting the project's own configuration to say 128 times what one section says better. `registry.rst` gains a sentence pointing at it, since every `register_*` there takes a code the enum must already resolve. * `CONTRIBUTING.md` records the tenet: what counts as contract, that per-option `_read_*` / `_make_*` pairs publish the data format and stay, that most class private attributes stay, that anything abstract or implemented across subclasses must be documented, and that the recipe to hide is chiefly module-level privates. Deliberately not changed: the `_read_*` / `_make_*` families, which outline the constructor contract and the data format for each option, parameter, chunk and cause; every other class private attribute; and the six class members that looked like helpers but each document observable behaviour a caller needs -- `_split_key` interprets caller-supplied keying material, `Vendor._request` is the hook a subclass with a different registry source overrides, and `PyPCAPFile._decode` and `_get_decoder` record the warnings they emit and the guarantee they make. No `pcapkit/` line changed, so no changelog entry, following #686. `sphinx-build -b html` exits 0 with 56 warnings against 55 on `f0999858e`. The one added is `more than one target found for cross-reference 'Type'`, raised from the newly rendered `TraceFlow._foutio`, whose doc comment at `pcapkit/foundation/traceflow/traceflow.py:406` reads `#: Type[Dumper]: Dumper class.` -- Napoleon turns that bare `Type` into a cross-reference that five classes in the tree answer to. The same warning already fires four times at `f0999858e` from `engine.rst`, `reassembly.rst` and `traceflow.rst`, so this is a fifth instance of a standing ambiguity rather than a new kind of breakage, and no reference was orphaned. Qualifying that annotation would silence it, but the file is outside the scope of this change. Fixes #684
JarryShaw
force-pushed
the
docs/private-directive-sweep-684
branch
from
September 23, 2026 04:50
dd53063 to
277bf47
Compare
Owner
Author
GOOD TO GOCross-review of the rebuilt change ( What it corrected
What it verified and did not dispute
Its own build reported 54 warnings rather than my 55/56 — it identified the cause itself as a missing optional One observation it made that I am recording rather than acting on
|
This branch has not been deployed
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
1 participant
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.
Reconciles the Sphinx API reference against the owner's tenet — document the contract, hide the recipe — in both directions.
Fixes #684
This branch was rebuilt from
f0999858e. An earlier revision implemented a different rule ("remove a private directive unless something public cross-references it") and removed 164 directives; that rule has been superseded, so the branch was reconstructed rather than patched, and the earlier kept/removed lists in the comments below no longer describe it. They are left in place because the cross-review was measured against them.What changed
autoattributedirectives addedconst/index.rst, a cross-reference inregistry.rst)pcapkit/lines changedNet: 13 files, +139 / −20.
Removed — 7, every one a module-level private
None is on any class's usage or extensibility surface: a private helper function, two lazily-imported backend flags, an internal lock, and a private stream adapter nobody constructs or subclasses.
autofunction:: pcapkit.protocols.internet.esp._resolveautodata:: pcapkit.protocols.internet.esp._CRYPTOautofunction:: pcapkit.protocols.application.ngap._convertautofunction:: pcapkit.protocols.application.ngap._revertautodata:: pcapkit.protocols.application.ngap._PYCRATEautodata:: pcapkit.protocols.application.ngap._PDU_LOCKautoclass:: pcapkit.foundation.engines.pypcapfile._NamedStreampypcapfile.py:232_NamedStream's two nested members (name,read) go with it. They are non-underscore, but they are that private class's members — they exist to satisfy the file-object protocolpcapfilerequires — and a member of a private class is reachable only through it. This is not the publicEngine.name: that is a hand-written.. property:: nameatdocs/source/pcapkit/foundation/engines/engine.rst:18and is untouched by this branch.Added — 39
autoattributedirectivesThe strongest cases are the ones that were alone in being omitted from a group whose siblings are all documented:
Extractor._flag_f— the only one of ten_flag_*attributes without a directive. Read directly by engines atpcapkit/foundation/engines/pcap.py:124.PCAP_CT._backend,PyPCAP._backend— every sibling on those classes (_expkg,_extmp,_handle,_dlink,_closed) is documented; these two were skipped, though_backend's own doc comment says "What thepcapimport actually found, c.f.:attr:backend`".3rdparty.rstcarry a private-state block;builtin.rstcarried none, soPCAP._gbhdr,_vinfo,_dlink,_nnsecandPCAPNG._ctx,_ctx_listare added to match.TraceFlow._fproot,_foutio,_fdpext,_endian,_nnsecd,_analyse— set in the base's__init__and read directly byTCP's overriddendump/submit(traceflow/tcp.py:274,:347,:446,:516). Their siblings_buffer/_stream/_bidirfrom the same__init__were already documented; this completes an incomplete copy.The rest either back a documented property or are written and read directly by collaborators:
Extractor._ifnm,_ofnm,_offmt,_magicbackinput/output/format/magic_number;_offmtis also assigned by an engine (ext._offmt = ofile.kind,pcap.py:130).Extractor._fext,_frame,_ipv4,_ipv6,_tcp,_eof_mark,_vfunc—ext.rst:504,:519,:539,:544already teaches engine authors to useext._extractor,ext._vfuncand friends by name, so they are documented API in prose while having had no entry in the reference.EngineBase._extractor— read asext = self._extractorby all eight concrete engines.SeekableReader._stream,_closed— backraw/closed, the same pattern as the already-documented engine attributes.ProtocolBase._next,_protos,_seekset,_sigterm—_next/_protosbackpayload/protocol/protochain;_seeksetis read by the@seeksetdecorator atutilities/decorators.py:74;_sigtermis read by subclass overrides atinternet.py:256andipv6.py:404.FieldBase._name,_template,_default,_callbackandField._length,_length_callback— the widest fan-out in the tree: reassigned directly byPayloadField,SchemaField,ListField,OptionFieldand every class innumbers.py,strings.pyandipaddress.py. Documented on the class that defines each, not on both, so nothing is stated twice.Added — the
_missing_contract, documented once_missing_has 128 runtime definitions — 121 underpcapkit/const/and 7 inline enums underpcapkit/protocols/— and had no directive anywhere. (A further 17 matches underpcapkit/vendor/are generator-side enums or the template text itself, not shipped enum classes.) It is the deliberate extensibility behaviour of the const enums per #647: looking a registry value up by an unassigned-but-in-range number mints a member rather than raising.It is documented once, as a new Unrecognised Values section on
docs/source/pcapkit/const/index.rst, with a cross-reference fromregistry.rst(where everyregister_*takes a code the enum must already resolve). Not 128 directives, for three reasons:@classmethods on their ownaenum.IntEnum/IntFlag/StrEnumsubclasses; nothing inpcapkit/supplies an enum base.conf.pyalready excludes it.docs/source/conf.py:115names_missing_inautodoc_default_options['exclude-members'], alongside__weakref__and_abc_impl. Per-class directives would be fighting the project's own configuration.pcapkit/vendor/default.py:53-112, so that is what the section describes.The rule, written down
CONTRIBUTING.mdgains What belongs in the API reference: document the contract, hide the recipe, replacing the section the earlier revision added for the superseded rule. It records that per-option_read_*/_make_*pairs are contract because they publish the data format, that most class private attributes stay, that anything abstract or implemented across subclasses must be documented, that the recipe to hide is chiefly module-level privates, and that dunders and:show-inheritance:-linked private bases keep their directives.Deliberately not changed
_read_*/_make_*families stay. Each pair is the published shape of that option, parameter, chunk or cause — the keyword arguments a caller passes and the fields they get back — and there is nowhere else to look it up. Being reached through agetattrdispatch table does not make it recipe.SecurityAssociation._split_key(how caller-supplied keying material is interpreted),Vendor._request(the documented low-level hook, with its proxy fallback, that a subclass with a different registry source overrides),PyPCAPFile._get_decoderand._decode(both record theAttributeWarningthey emit and the non-mutation guarantee they make),MH._seconds(the acceptedint-or-timedeltainput shape for MH interval fields) andMH._decode_signed(why bit-packed signed fields need reinterpreting).SeekableReader's nine buffer attributes —_closing,_tell,_buffer,_buffer_view,_buffer_size,_buffer_set,_buffer_cur,_buffer_path,_buffer_file(pcapkit/corekit/io.py:51-78), the same declaration block as the two that were added. Nothing outside the class touches them: they are the internal sliding-window mechanics, with no subclass and no external caller, whereas_streamand_closedback the documentedrawandclosedproperties. Recipe, so omitted deliberately rather than overlooked.hip.rst— owned by the in-flight HIP work (R1_COUNTER packs 12 octets where RFC 7401 §5.2.3 requires 16:counteris 4 octets, not the stated 8 #672 / LOCATOR_SET is conformant only by accident: nested Locator.len shadows the parameter in padding, and len is in 4-octet units where RFC Length is bytes #679).docs/source/changelog/— owned by docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657.Verification
Built in the project's own mode,
sphinx-build -b htmlwith no-W(becausemake docsdoes not use it), run from the worktree root withPCAPKIT_SPHINX=1so the branch's ownpcapkitis imported rather than the editable install; the build printedDOCUMENTING: …/pcapkit/__init__.pyunder the worktree to prove which tree it read.The one new warning, in full:
It comes from newly rendering
_foutio, whose doc comment atpcapkit/foundation/traceflow/traceflow.py:406reads#: Type[Dumper]: Dumper class.— Napoleon reads that bareTypeas a cross-reference and five classes in the tree answer to it. The same warning already fires four times onf0999858e(fromengine.rst:39,reassembly.rst:32,reassembly.rst:42andtraceflow.rst:39), so this is a fifth instance of a standing ambiguity, not a new kind of breakage, and nothing was orphaned.Qualifying that annotation to
typing.Type[Dumper]would silence it, but the file ispcapkit/source and outside this change's scope — flagged rather than fixed. The alternative, dropping the_foutiodirective, would hide a contract attribute to keep a warning count flat, which is the wrong trade.util/changelog_md.py --checkexits 0; no changelog entry, following #686, since zeropcapkit/lines changed.