fix(exports): export the 23 public attributes their package __all__ omitted (#533) - #544
Conversation
…mitted (#533) * `pcapkit/__init__.py` mirrors `pcapkit.protocols.__all__` again, restoring the 12 protocol names it drifted behind: `C_Tag`, `S_Tag`, `DRARP`, `InARP`, `L2TPv2`, `HTTPv1`, `HTTPv2`, `PCAPNG`, `Header`, `Frame`, `Data`, `Schema`. #436 updated the child list and left the parent alone. * `pcapkit/foundation/__init__.py` lists the six registry functions it withheld while listing the other 27, plus the two manager classes. #527 fixed the same six one level down, in `registry/__init__.py`. * `pcapkit/foundation/reassembly/__init__.py` and `traceflow/__init__.py` export `ReassemblyManager` and `TraceFlowManager`, defined below the `__all__` literal that forgot them and imported by name from six modules. * `pcapkit/utilities/__init__.py` exports `beholder`, `prepare` and `seekset`, which its own module docstring advertises and which pylint reported as unused imports precisely because no `__all__` named them. * `tests/project/test_public_api.py` gains the converse assertion over all 52 public packages against `DELIBERATE_NON_EXPORTS`, plus a test that stops that allowlist rotting into a silencer. Nothing outside an export list changed, and nothing reads the five lists as data. Unit tier 1036 passed / 8 skipped / 2536 subtests, with one pre-existing docstring-registry failure that reproduces identically on 122d327. pylint drops three messages and adds none.
|
✅ GOOD TO MERGE — independently re-derived the export-gap count from scratch with my own AST/introspection script (not copied from the PR's test code) and got an exact match at every stage: 45 unlisted names across 12 packages on |
Detailed review (independent verification, falsify-not-bless)Head sha reviewed: The core count -- re-derived from scratch, not copied from the PR's own scannerI wrote my own script (not the PR's
Fails-without proof -- reproduced, with one cosmetic tallying difference notedReverted the five source files to Also separately provoked The abstract-base exclusion argument -- two corrections found, conclusion unaffectedThis is the PR's single biggest judgement call (10 of the 22 residual names), so I checked all three of its supporting claims independently:
Net: the exclusion of all 10 abstract bases still reads as correct to me -- the surviving evidence (8 packages via a comment convention plus 2 independent corroborations for
|
|
Flagging a conflict with this PR's biggest judgement call, so it is visible where the merge decision gets made rather than only on another thread. This PR deliberately withholds ten abstract bases from their package On #514 the owner has just disagreed. Asked whether his answer overturned this, he replied (#514, 2026-09-20T07:23:52Z):
His earlier answer in the same thread pointed the same way — "what public classes? If it's the layer's base classes, those are needed." I am not proposing an edit to this PR and have not touched it. Two notes that may bear on how it is resolved:
Whichever way it goes, the allowlist reasoning in this PR is the right place for it to be recorded — that is what makes the conflict legible at all, rather than the names simply staying absent. |
|
Follow-up to the above, now measured rather than flagged: one of the owner's two justifications is not a justification.
So |
Closes #533.
The count, re-derived
The issue's figure is "roughly 45 public attributes across about 12 non-aggregator packages", and says plainly that it "has not been independently re-derived". It has been now, with a scanner rather than by hand, and it is exactly 45 across exactly 12 packages — measured on
122d32795, the base this branch was cut from, i.e. after #527.Definition used, matching the one
test_aggregators_export_every_public_attributealready applies to the three aggregators: for each of the 52 public packages, every attribute of the package that does not start with_, is not named in that package's__all__, and is not a module object. Submodules are excluded because a submodule becomes an attribute of its parent as a side effect of being imported, so counting them would flag every package that has children.0f4beeff6(the #527 merge)122d32795(this branch's base)#527 had already handled its own share, and none of my 45 overlap with it. #527 fixed the inverse defect —
__all__entries naming nothing — in six files, and as a side effect brought the three aggregators to zero unlisted attributes. That is why all three aggregators are absent from the table above: measured at the #527 merge commit they already contribute nothing, so there is no double-counting and nothing to exclude. The 45 are a disjoint set from #527's work.One correction falls out of this. #527's own module docstring records the figure as "45 across 13", and that is the number the issue paraphrased as "about 12". Measuring the #527 merge commit directly gives 45 across 12: the name count was right and the package count was one high. The docstring is corrected in this PR as part of rewriting that paragraph.
What was added: 25 entries, closing 23 of the 45
Triage, not a mechanical sweep. A name was added only where the package's own established pattern is broken by its absence — never merely because the name is public and documented.
pcapkit/__init__.pyC_Tag,S_Tag,DRARP,InARP,L2TPv2,HTTPv1,HTTPv2,PCAPNG,Header,Frame,Data,Schema__all__is a per-layer mirror ofpcapkit.protocols.__all__, and it is complete for the internet and transport layers. All 12 are inpcapkit.protocols.__all__already; 9 of the 12 are also inpcapkit/all.py, the "Library Index" whose stated job is to hold "all things frompcapkit". The drift is datable:git log -S"C_Tag" -- pcapkit/__init__.pyreturns nothing at all, while #436 (f50436a8a) addedC_Tag/S_Tag/L2TPv2topcapkit/protocols/__init__.py,pcapkit/protocols/link/__init__.pyandpcapkit/all.py— and left the top level alone.pcapkit/foundation/__init__.pyregister_*functions, plusReassemblyManager,TraceFlowManager__all__re-lists the registry's exports and names 27 of the 33. The 6 it withholds are precisely the 6 that #527 added one level down topcapkit/foundation/registry/__init__.py, for the reason #527 gave: each carries an.. autofunction::directive atdocs/source/pcapkit/foundation/registry.rst:32-45. Four of the six live in the same module asregister_extractor_engine,register_dumper,register_extractor_dumperandregister_traceflow_dumper, all of which are listed. The two manager entries are the cascade from the two rows below —pcapkit/foundation/__init__.pywildcard-imports both packages.pcapkit/foundation/reassembly/__init__.pyReassemblyManager__all__literal that omits it (__init__.py:38, literal at:24), documented atdocs/source/pcapkit/foundation/reassembly/index.rst:52under the package path, and imported by name fromfoundation/extraction.pyand four engine modules.pcapkit/foundation/traceflow/__init__.pyTraceFlowManager:37, literal at:23, documented atdocs/source/pcapkit/foundation/traceflow/index.rst:53, imported by name from three modules.pcapkit/utilities/__init__.pybeholder,prepare,seeksetseeksetandbeholder".pcapkit/utilities/decorators.py:36exports all three. All three carry.. autodecorator::directives atdocs/source/pcapkit/utilities/functools.rst:15-19. And pylint was already saying so — see the lint section.25
__all__entries, closing 23 of the 45. The arithmetic:45 − 23 = 22residual, all allowlisted. The extra 2 entries areReassemblyManager/TraceFlowManagerappearing a second time, inpcapkit/foundation, where they became visible only because of the two rows above — a cascade the scanner caught on the second pass, not a name from the original 45.Nothing outside an export list changed. No module body, no import, no docstring in
pcapkit/. And nothing reads any of the five lists as data: the only two consumers of an__all__as a value arepcapkit/protocols/__init__.py:74(thefor name in __all__that builds__proto__) andpcapkit/vendor/__main__.py:79,87(the crawler target list), and this PR touches neither file. Worth stating because it is a live trap rather than a hypothetical: adding'Protocol'to the first__all__literal inpcapkit/protocols/__init__.pywould have registered aPROTOCOLentry in__proto__.Protocolis not exported here for other reasons, but that is the shape of the accident this rule avoids.Names deliberately NOT exported — 22, and why
These are in the new
DELIBERATE_NON_EXPORTSallowlist with the reasoning written next to them, which is what the issue asked for: "consider whether a leading underscore would express the intent better than silence, since silence is indistinguishable from an oversight". Recording them is the compromise — the intent becomes reviewable without touching a module body.The 10 abstract bases —
Field,Engine,Reassembly,IP_Reassembly,TraceFlow,Protocol,Application,Internet,Link,Transport. Every one is documented with anautoclass::directive, which is the signal #527 used, so the case for exporting them is not weak. They are left alone anyway, because three independent export surfaces agree in excluding them:# Base Class for ...comment and each omits it from__all__, the same way;pcapkit/all.py— the library index — lists no base class at all;pcapkit/corekit/__init__.pylists the 22 concrete*Fieldclasses and notField, and refers to it atcorekit/__init__.py:15as:class:`~pcapkit.corekit.fields.field.Field`— by its defining path, which is the docs convention here.A pattern repeated nine times and corroborated by two other surfaces is a convention, not nine oversights. If the owner disagrees, this is the judgement call to overturn — it is 10 of the 22 and the single biggest decision in the triage. Deleting the relevant allowlist entries and adding the names is then a mechanical follow-up.
7 names another module's import dragged in —
Infoandinfo_finalin bothpcapkit.foundation.reassemblyandpcapkit.foundation.traceflow(they belong topcapkit.corekit.infoclass, which documents and exports them; the two packages import them only to build their manager classes), andTYPE_CHECKINGin three packages, which istyping's. Re-exporting any of these would claim a name the package does not own.4 pieces of import-time machinery at the top level —
BaseError,DEVMODE,DevModeWarning,ROOT.pcapkit/__init__.py:82-94imports the first three solely to wire up thetbtrimexcepthook; they are documented underpcapkit.utilities.ROOTis the sharpest of the four:pcapkit/__init__.py:92binds it only on theelsebranch, so exporting it would makefrom pcapkit import *raiseAttributeErrorunderPCAPKIT_DEVMODE=1— manufacturing a fresh #515 while claiming to fix one.BaseErroris the most arguable of the four, being the exception a user would naturally want to catch; it is left out because the top-level surface is interface functions, macros and protocols, and it is exported frompcapkit.utilities.exceptionswhere it is documented.1 leaked loop variable —
pcapkit.protocols.name, left behind by thefor name in __all__:atpcapkit/protocols/__init__.py:74. Renaming it_namewould express the intent better than an allowlist entry does, and the issue invites exactly that; it is a change to a module body rather than to an export list, so it is flagged here for the owner instead of made silently. One line, no behavioural effect.Two more judgement calls that are not allowlist entries, because the scanner does not count module objects:
dataandschemaas submodule names inpcapkit.__all__. Bothpcapkit.protocols.__all__andpcapkit/all.pyexport the pairData/dataandSchema/schematogether. This PR adds onlyDataandSchema, leaving the mirror one step short. Exporting a module object through__all__is a different kind of decision from exporting a class, and it is outside both the issue's scope and the new test's, so it is the owner's call.pcapkit/all.pyis not touched, see below.Found but not fixed:
pcapkit/all.pyhas the same defect, module-scopedpcapkit/all.pyis a module rather than a package, so it falls outside the issue's scope, the scanner's, and the new assertion's. It holds 16 unlisted public non-module attributes against its 151-entry__all__:They are not all the same thing. The last eight are the whole of
pcapkit.utilities.__all__, andall.py:135-136shows that omission is deliberate — a# pcapkit.utilitiesheading followed by a commented-out line. ButESP,PCAPNG,HTTPv1andHTTPv2look like the same drift this PR fixes at the top level:ESPis in bothpcapkit.__all__andpcapkit.protocols.__all__and has been since #378, yetfrom pcapkit.all import *does not provide it.ModuleDescriptor,MultiDict,OrderedMultiDictandinfo_finalare inpcapkit.corekit.__all__and missing here too.Not fixed in this PR for two reasons:
all.pyis not a package__init__.py, and its utilities section shows it carries deliberate omissions of its own, so triaging it properly is a separate pass. Worth its own issue. Note that the three decorators this PR adds topcapkit.utilities.__all__flow intopcapkit.all's namespace and are not added to its__all__— consistent with the whole utilities group already being excluded there, rather than a new gap.The test: extended, not duplicated
tests/project/test_public_api.pyis #527's module and this extends it, which is the cleaner fit — it already owns the package walk, the exclusion rule and the__all__invariants, and its docstring is where the scoping decision that created this issue was recorded.Three things change:
test_every_public_package_exports_every_public_attribute— the converse assertion over all 52 public packages, subtractingDELIBERATE_NON_EXPORTS. This is the "second contract assertion in the other direction" the issue suggested.test_the_non_export_allowlist_is_tight— the allowlist is load-bearing, so it is itself asserted. Three kinds of rot each fail: an entry naming a package not in the walk, an entry naming an attribute the package no longer has, and an entry naming something the package now exports anyway. Without this, a stale entry silently excuses the next real omission of the same name.test_aggregators_export_every_public_attributeis kept rather than folded in, because its guarantee is strictly stronger: it admits no allowlist at all. Both now share one_unlisted_public_attributeshelper, so the two assertions cannot drift apart in what they consider a public attribute.The docstring paragraph that scoped the converse to the aggregators is rewritten, since that scoping is what this PR changes. The "2848 names across 286 modules" measurement stays — it is still the reason the assertion is package-scoped rather than module-scoped.
Fails without the fix
Reverting only the five
pcapkit/files and keeping the test (git apply -Rof the source half of the diff):One subtest per file changed, and each names exactly the omission it was written for — 12 + 6 + 1 + 1 + 3 = the 23:
With the fix restored:
10 passed, 432 subtests passed, EXIT=0.Note what that output would have hidden. pytest 9.1.1's native subtests print the parent as
PASSEDeven when subtests fail — the run above reads "10 passed" while five subtests failed. The exit code is the only honest signal, so every number here was read from a file (print $? > …) rather than from a pipe.test_the_non_export_allowlist_is_tightgets its own fails-without, since an assertion that cannot fail is worth nothing. Each branch was provoked separately:'pcapkit.no.such.package': ('Whatever',)['pcapkit.no.such.package'] != []— names packages that are not in the public walk'GoneAwayLongAgo'to thepcapkitentry{'pcapkit': ['GoneAwayLongAgo']} != {}— names attributes these packages do not have'NoPayload'to thepcapkitentry{'pcapkit': ['NoPayload']} != {}— names attributes these packages now export anywaySphinx: 3 duplicate-object warnings before, 3 after
Full builds of
docs/at base122d32795, one without the change and one with:duplicate object description122d32795, pristine122d32795+ this changeThe 3 are the known pre-existing ones, unchanged:
pcapkit.corekit.infoclass.Info.__post_init__,pcapkit.foundation.reassembly.data.tcp.BufferID,pcapkit.protocols.misc.pcap.header.Header.__post_init__. The other 47 are 31more than one target found for cross-reference(TCP×13,UDP×6,type,_AT,_IT,_DT,_BT,Echo,Type) and 16 artefacts ofpcap/pcapfilenot being installed, plus two stray prints from scapy. None of the 50 names any of the 23 exported names.That the count could not have moved is also true by construction, and cheaper for a reviewer to check than a build is to run:
docs/source/conf.py:118sets'ignore-module-all': True, so autodoc ignores__all__outright when selecting members, and there is noautomoduledirective anywhere underdocs/— every autodoc directive in the tree is an explicitautoclass::/autofunction::/autoexception::/autodecorator::naming a fully-qualified object. An__all__-only change therefore cannot alter what Sphinx documents. The builds confirm the argument rather than replacing it.Lint: three pylint messages removed, none added
Running the project's own pylint invocation over the five changed files:
wrong-import-order, andunused-importforbeholder,prepare,seeksetwrong-import-orderThe two
wrong-import-orderare pre-existing, on thefrom typing import TYPE_CHECKINGthat sits below__all__in the reassembly and traceflow packages; this change only shifts those lines by one.The three that disappear are the interesting ones. With
--init-import=yes, pylint reportedbeholder,prepareandseeksetas unused imports inpcapkit/utilities/__init__.pybecause no__all__named them — the linter had been reporting this particular omission all along, in the one form that reads as a complaint about the import rather than about the export. Adding them to__all__is what makes the import purposeful, and it is independent corroboration that those three were an oversight rather than a decision.isort does not apply:
Makefile:125andcron-vendor.yml:79both pass--skip-glob '**/__init__.py', and all five changed files are__init__.py.Suite
CI's unit-tier selection, verbatim from
unit-tests.ymlandtests/_tiers.py:134:On the rebased tree:
1038 passed, 8 skipped, 2540 subtests passed, EXIT=0. Fully green.Run first on base
122d32795, it was1036 passed, 8 skipped, 2536 subtests passedwith one failure —test_docstring_contract.py::DocstringParameterTests::test_known_defects_are_still_defects, on thepcapkit/vendor/ipx/packet.pyentry. Confirmed pre-existing rather than assumed: reverting this branch's entire diff reproduces it identically on pristine122d32795(1 failed, 5 passed, 9 subtests passed, EXIT=1). #538 has since fixed it onmain, and rebasing onto that clears it.One commit, rebased onto
origin/mainat691f12ab5.Every measurement was made with
PYTHONSAFEPATH=1 PYTHONDONTWRITEBYTECODE=1andPYTHONPATHpinned to this worktree, withpcapkit.__file__asserted and printed to be inside it — the venv haspcapkitinstalled editable against a different checkout, which would otherwise shadow it and produce a plausible, wrong list: