Repository navigation
fix(protocols): repair __all__ entries that name nothing, and guard the class - #527
Conversation
…he class `from pcapkit.protocols.data import *` raised AttributeError on 'HOPOPT_QuickStartOption' -- an __all__ entry that was never bound. Broken since 1831bb6 (2023-03-06) split QuickStartOption into Request/Report variants and updated the child module but not the aggregator above it. * pcapkit/protocols/data: spell the two HOPOPT QuickStart variants that actually exist; add the missing 'Data' and 'ESP' re-exports. * pcapkit/protocols/schema: add the 18 omitted HOPOPT* names, plus 'ESP', 'Schema', 'EnumSchema' and 'schema_final'. * pcapkit/foundation/registry: add the 6 registration functions that carry autofunction directives in the docs but were absent from __all__. * pcapkit/const/pcapng: __all__ was spelled ___all__ (three underscores), so the package had none and star-import leaked its 7 submodules. * pcapkit/const/mh, pcapkit/const: 'MH_HandoverACKStatus' appeared twice, displacing 'MH_HandoverInitiateStatus'; import that enum and export it. tests/project/test_public_api.py is new and covers the whole class: every public package survives `import *`, every __all__ entry resolves, none is misspelled or duplicated, and the three aggregators list all they re-export. Fixes #515.
ad814dc to
2b5886c
Compare
|
❌ NEEDS CHANGES — |
Detailed review (independent verification, falsify-not-bless)Reviewed at head 1. Vendor-side fix — confirmed NOT done, two ways.
2. 3. General 4. The contract test — found, read, verified both directions with real commands. File: 5. isort claims verified, including that 6. "Guard the class" identified and confirmed load-bearing. It's the 7. A real gap found beyond the checklist, disclosed but worth tracking. The converse assertion ("every public attribute is in VerdictNEEDS CHANGES: fix |
Response to NEEDS CHANGES: respectfully disputing the blocker, with evidenceThank you for the review — findings 2-6 independently reproduce what this PR claims, and finding 7 is a real gap I had not measured. On the blocking item I disagree, and I think the premise is factually out of date rather than wrong in reasoning. The vendor-side defect is already fixed, in #531 — open, Title: "fix(vendor): make the handover_initiate_status crawler reachable (release blocker for It also carries corroborating evidence I had not found: So the release blocker is closed; it is just not closed here. That was deliberate and is disclosed in this PR's own out-of-scope section, which is how the review found it. Why making the change in this PR would be actively harmfulThe two PRs are exactly file-disjoint — verified with
Both PRs would have to modify the same single line — On the substance of "the const side is downstream of the broken one"Agreed, and worth stating precisely, because it argues for two PRs rather than one. So: #531 unblocks RequestedRe-review this PR on its stated scope. If the preference is instead that both land together, the clean way is to gate merge order (#531 then #527, or the reverse — they do not interact), not to duplicate the same two lines across both branches. Finding 7 — accepted, not fixed hereThe 45 public attributes omitted from non-aggregator |
|
✅ GOOD TO MERGE (supersedes my earlier ❌ NEEDS CHANGES) — the vendor-side |
Correction to my earlier reviewMy prior detailed comment on this PR raised Finding 1 as a NEEDS CHANGES blocker: that Why this PR shouldn't touch those files: PR #531 fixes exactly Why the const-side fix in this PR is not "downstream and moot" — it stands on its own. My original framing treated the const-side Findings 2 through 6 — unchanged, still creditedThe Finding 7 — not a defect in this PR, tracked separatelyThe 45 public attributes across roughly a dozen non-aggregator packages that are omitted from their Revised verdictGOOD TO MERGE. The release-blocker mechanism is closed by #531 in a properly partitioned, disjoint PR; this PR's own const-side fix is independently correct and necessary regardless of #531's timing. |
…ease blocker for `make vendor`) (#531) * fix(vendor): make the handover_initiate_status crawler reachable `pcapkit/vendor/__main__.py` builds its crawler target list out of `__all__` rather than by discovery, at line 87 for a bare `pcapkit-vendor` and line 79 for `--target <name>`. Both MH lists named 'MH_HandoverACKStatus' twice and had no entry for 'MH_HandoverInitiateStatus', so 117 crawlers were defined and only 116 could ever run. - pcapkit/vendor/mh/__init__.py: import HandoverInitiateStatus, which was never imported into the package at all, and replace the duplicate `__all__` entry with it. The missing import is the second half of the defect: repairing `__all__` alone would have raised AttributeError. - pcapkit/vendor/__init__.py: replace the same duplicate `__all__` entry. No import needed -- it wildcard-imports pcapkit.vendor.mh. - Both docstring tables listed MH_HandoverACKStatus against the text "Handover Initiate Status Codes"; corrected to match the footnotes. - docs/source/pcapkit/const/mh.rst: the same displaced row, so the .rst agrees with pcapkit/const/mh/__init__.py as corrected by #527. - tests/vendor/test_crawler_reachability_unit.py: assert every Vendor subclass under pcapkit/vendor/** is reachable from both target-list paths, and that no `__all__` repeats an entry. The constant file is deliberately not regenerated; that needs network access and belongs to the pre-release `make vendor` run. isort and mypy clean; pylint reports no message the two files did not already report on main. 5 new tests pass, 4 of them fail without the fix. * docs: correct the displaced handover-status row in the vendor mh table The first commit fixed this row in `pcapkit/vendor/mh/__init__.py`'s docstring table, which left `docs/source/pcapkit/vendor/mh.rst` disagreeing with it by one row -- the same divergence in the vendor tree that the `const/mh.rst` change in that commit closes in the const tree. Line 53 named MH_HandoverACKStatus against the text "Handover Initiate Status Codes", so MH_HandoverACKStatus appeared twice and MH_HandoverInitiateStatus not at all. The footnote list underneath was already ordered #handover-initiate-status then #handover-acknowledge-status, so correcting the row makes the table agree with its own footnotes. The per-crawler body section further down the file was always correct -- it already carries `.. autoclass:: pcapkit.vendor.mh.handover_initiate_status.HandoverInitiateStatus` -- so only the summary table needed the change.
…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.
…mitted (#533) (#544) * `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.
1 participant
Fixes #515.
The defect
from pcapkit.protocols.data import *raisedAttributeErroronHOPOPT_QuickStartOption— an__all__entry that was never bound.1831bb60b(2023-03-06) splitQuickStartOptioninto Request/Report variants and updated the intermediatepcapkit/protocols/data/internet/__init__.py, but not the aggregator above it that re-lists the same names under the same prefixes.Broken for ~3.5 years. A name in
__all__is inert: it is read byimport *and by Sphinx, and by nothing the suite exercised. Verified thatHOPOPT_QuickStartOptionexists nowhere in the tree, while the intermediate packages do export both real variants:What changed
Six
__init__.pyfiles, all the same defect class — an__all__that disagrees with what the module actually holds.pcapkit/protocols/data/__init__.pyHOPOPT_QuickStartOption; also missingDataandESPpcapkit/protocols/schema/__init__.pyHOPOPT*names while listing all 18IPv6_Opts*equivalents; also missingESP,Schema,EnumSchema,schema_finalpcapkit/foundation/registry/__init__.py.. autofunction::directives atdocs/source/pcapkit/foundation/registry.rst:32-45pcapkit/const/pcapng/__init__.py___all__, with three underscorespcapkit/const/mh/__init__.py'MH_HandoverACKStatus'listed twice, displacing'MH_HandoverInitiateStatus'pcapkit/const/__init__.pyconst/mhEvery newly-exported name was checked to come from a child that already exports it, which is the aggregator contract.
The two folded-in defects were verified independently, not taken on trust
___all__inconst/pcapng. Nothing reads that name, so the package had no__all__:hasattr(module, '__all__')wasFalseandimport *fell back to "every public name", leaking its 7 submodules on top of the 7 enums it meant to publish. This is the quietest member of the family — a misspelling here does not raise, it silently widens the public surface. It is the only occurrence of the typo in the repo. Confirmed no code depended on the leaked submodule names.The displaced duplicate in
const/mh. A duplicate is harmless in itself, which is why it survived; what made it a defect is what it displaced.MH_HandoverInitiateStatuswas never imported into the package, sopcapkit.const.mh.MH_HandoverInitiateStatusdid not resolve at all — even though the enum is real and actively used bypcapkit/protocols/internet/mh.py,data/internet/mh.pyandschema/internet/mh.py, which reach past the package to the leaf module. Fixing__all__alone would have introduced a fresh #515, so the import is added too. The docstring summary table had the same duplicated row (it namedMH_HandoverACKStatusagainst the text "Handover Initiate Status Codes"); its cross-reference now resolves to the module that is already documented atdocs/source/pcapkit/const/mh.rst:387.Both are the same defect class as #515 and one reviewer pass covers all three, which is why they are here rather than in a follow-up.
The test
tests/project/test_public_api.py(new) covers the whole class rather than the three instances. Six assertions, over all 325 public modules / 52 public packages:from <pkg> import *statement;__all__entry resolves (hasattrsweep, reported as a complete list);__all__entry is a non-string;__all__-lookalike (___all__,__all_, …), which nothing reads;__all__— the "forgot it" counterpart to "typed it wrong";__all__names the same symbol twice. A duplicate is invisible to the resolve sweep, because the repeated name does resolve; only counting entries finds it.The converse — every public attribute appears in
__all__— is asserted only for the three aggregators, and the module docstring records the measurement behind that scoping: applied to every module it flags 2848 names across 286 modules, and most are not omissions but names a wildcard dragged in (TYPE_CHECKING,Info,info_final). A blanket assertion would be a wall of false positives and an invitation to silence it by adding junk to__all__. On the three pure re-export surfaces it holds exactly, at zero.pcapkit.vendoris excluded, with the reason documented in the module: its modulesimport requests/bs4from thevendorextra, whichpip install -e '.[test]'does not install — and that is exactly how CI runs this tier. Little is hidden by it, and what is hidden was measured rather than assumed: the same sweep was run by hand overpcapkit.vendoron a machine that does have the extra — the package plus its 134 public descendants. Every one imports, every one declares__all__, and every__all__entry resolves. The only defect in that subtree is the duplicate described below. No import is wrapped in a skip, so an import failure in the sweep is a real defect and is allowed to fail.Fails without the fix
Reverting only the six source files and keeping the new test:
Each new assertion names precisely the defect it was written for:
With the fix restored:
Wider targeted run,
tests/const tests/project tests/interface:41 passed, 500 subtests passed, exit 0. Measurements made withPYTHONSAFEPATH=1 PYTHONDONTWRITEBYTECODE=1andPYTHONPATHpinned to this worktree, withpcapkit.__file__asserted to be in it — the venv has pcapkit installed editable against a different checkout and would otherwise shadow it.isort -l100 -ppcapkit --check-onlypasses onconst/mh/__init__.pyandconst/pcapng/__init__.py, which is the pathcron-vendor.ymlisorts.pcapkit/const/__init__.pyfails that check, but fails identically on unmodifiedorigin/mainand is not a path CI isorts, so it is left alone.Out of scope — one of these is a release blocker
pcapkit/vendor/__init__.pyandpcapkit/vendor/mh/__init__.pycarry the same displaced duplicate, and there it is worse than cosmetic.pcapkit/vendor/__main__.py:87builds the crawler target list frompcapkit.vendor.__all__, andcron-vendor.ymlinvokes barepcapkit-vendor— the default path. SoMH_HandoverInitiateStatusis absent from the default target list and from--target mh, andpcapkit/vendor/mh/handover_initiate_status.pyhas therefore never run. Not fixed here: those files are outside this change's ownership, and testing them needs thevendorextra, which this tier does not have. When that lands, drop'vendor'fromEXCLUDED_ROOTSand this module covers it too. The test docstring records this so it is not lost.Also left alone:
docs/source/pcapkit/const/mh.rst:53-55mirrors the duplicated summary-table row that this PR fixed in the docstring, so the two now disagree by one row; and the pre-existingisortdeviation inpcapkit/const/__init__.py.