Skip to content

refactor: import each *Base class under its own name, not the public one - #750

Merged
JarryShaw merged 1 commit into
mainfrom
fix/514c-alias-rename
Sep 24, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/514c-alias-rename

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 24, 2026 •

Copy link
Copy Markdown
Owner

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

Part of #514. Library modules imported a *Base class under its public name — from … import EngineBase as Engine, then class PCAP(Engine) — so the source read as inheriting the public class while actually inheriting the base. Per the ruling the split is permanent and library classes inherit the base, so the base is now imported under its own name.

Partial, and the boundary is file ownership rather than judgement:

family sites shipped deferred
EngineBase as Engine 8 8 —
ReassemblyBase as Reassembly 3 3 —
DumperBase as Dumper 2 2 —
TraceFlowBase as TraceFlow 2 2 —
ProtocolBase as Protocol 67 13 54
total 82 28 54

All four non-protocols families are complete. Every one of the 54 outstanding is a ProtocolBase site in a path an open PR owns — pcapkit/protocols/ and foundation/registry/protocols.py (#726), foundation/extraction.py and foundation/traceflow/traceflow.py (#742) — held as four path prefixes in PENDING_ALIAS_PATHS, which fails once a prefix stops matching, so the promise cancels itself as those land.

The issue's 82 is right; I measured 83 first and was wrong — EngineBase as Engine is 8, not 9. Split: 53 TYPE_CHECKING, 24 module-scope, 5 function-local. FieldBase/Field excluded — not registration-motivated, and unlike the five it has 26 descendants, so its alias is abbreviation, not misstatement.

No behaviour change, measured not asserted: name registry 38 keys before and after; descendants(Public) 0 in all five suites; 45 real dispatches identical, and still identical after __proto__.clear() — confirming the built-in ModuleDescriptor tables are populated independently of name registration, as you said; two captures parse byte-identically; mypy at exactly its 112-error baseline and pylint at exactly its message profile, 0 introduced either way.

One thing #514 asks for that I did not build, and want a ruling on. The ruling also says __init_subclass__ should check against Protocol specifically. Measured, that check is a tautology in all five suites: the registering hook is defined on the public class, so it only runs when the public class is in the new class's MRO — issubclass(cls, Public) is true every time it runs, and it never runs otherwise. Library classes are already excluded by inheritance, which is stronger than a runtime test. And the two hooks that are reachable from library classes must not get the guard: ProtocolBase.__init_subclass__ assigns __schema__/__data__ for all 43 built-ins, and the ReassemblyBase/TraceFlowBase hooks initialise __callback_fn__. So the discrimination is pinned as tests rather than added as dead code — say the word if you want it spelled out anyway.

#682 looks dissolved, not merely deferred: all three dispatchable HTTP classes are ProtocolBase subclasses, so under a public-only rule none would register, and import pcapkit emits zero RegistryWarning (the only warning is third-party VueJS is deprecated). HTTP is claimed once, by the __all__ walk. What survives is narrower — the key is a shared namespace, so a user class named HTTP subclassing Protocol would still displace it.

Not breaking. All five public classes and every registration path are untouched, so an out-of-tree subclass registers exactly as before. The only observable loss is 14 modules no longer binding the public name incidentally, and none of the 24 such modules declared it in __all__ — never public API, and it fails as a loud ImportError rather than quietly meaning something else.

Deliberately excluded: 10 RegistryError messages say "must be a Protocol subclass" while checking the base; 8 sit in #726/#742 files, and fixing 2 of 10 splits the wording two ways for one check. Worth one pass once those land. (The issue says eleven; two of the twelve register_dumper sites correctly mean third-party dictdumper.dumper.Dumper.)

On splitting: one PR is right, because this turned out not to be the risky change. The rename produces identical class objects — class Link(Protocol) where Protocol is ProtocolBase compiles to exactly what class Link(ProtocolBase) does — so the #514 invariant holds at every intermediate step and there is no half-migrated state to protect against. That hazard belongs to the original collapse, not to this. Which leaves reviewability, arguing against splitting: it is one transformation rule, checked once. Per-suite PRs would be four reviews of 2–8 lines each.

Also corrected four docs/source/**/index.rst pages that said "All X are implemented as <Public> subclasses" while their own Mermaid diagram three lines below correctly roots the hierarchy at *Base; corekit/fields/index.rst already named the base and was the model.

Tests. tests/test_base_class_contract.py is new — 7 tests / 21 subtests, at the tests/ root because the contract spans four suites so no suite directory owns it. Its two alias tests fail on unmodified main (28 source sites in 21 files; 14 runtime bindings) and pass here; the five RegistrationGateTests are regression pins for what parts (a)/(b) already gave, including that a library-style subclass cannot opt in at all — class X(EngineBase, engine=…) is a TypeError, the final substitute you described. Ran tests/{corekit,dumpkit,foundation,interface,utilities}, tests/protocols/internet/test_ipv4_unit.py and the new file: 625 passed, 11 skipped, 2546 subtests, 0 failed (fixtures built with examples/generators/make_samples.py; without them 34 fixture-dependent tests fail on any tree, and all 43 pass once built). The five test_ipv4_unit.py assertions pinning Protocol is not ProtocolBase all still pass. Coverage on the same selection rises 45% → 48% (1639 more statements, identical 40383 denominator — the rename adds no library statements).


Body corrected after cross-review: 21 files not 22, and 24 module-scope alias sites not 20 (the reviewer's count includes DumperBase as Dumper in dumpkit/null.py:18 and dumpkit/pcap.py:16, both plain module scope, which my four-family sweep had excluded). Conclusions unchanged — 0 of all 82 sites put the public name in __all__.

Library modules imported a *Base class aliased to its public name, so the
source read `class PCAP(Engine)` while actually inheriting `EngineBase`. Per
the ruling on #514 the split is permanent and library classes inherit the
base, so the base is now imported under its own name.

* rewrote 28 of the 82 alias imports -- EngineBase 8 of 8, ReassemblyBase 3
  of 3, DumperBase 2 of 2, TraceFlowBase 2 of 2, ProtocolBase 13 of 67
* renamed the code and annotation references that followed, including
  `cast()` targets, `TypeVar(bound=)` and `# type:` comments, but not the
  string values inside `Literal[...]`
* corrected four `docs/source` index pages that named the public class while
  their own class diagram roots the hierarchy at the base
* added `tests/test_base_class_contract.py`, pinning the contract per suite

The four non-protocols families are complete. The remaining 54 sites are all
`ProtocolBase` ones in paths #726 and #742 own, recorded as prefixes in
`PENDING_ALIAS_PATHS` so the entry fails once its pull request lands.

No behaviour change: name registry 38 keys before and after,
`descendants(Public)` 0 in all five suites, 45 dispatches identical and still
identical after clearing the name registry, mypy at its 112-error baseline
with none introduced.
@JarryShaw JarryShaw added refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 24, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE @ 8ef38142d — rename verified byte-identical pre/post on every metric checked (MRO, bases, registries, mypy, __all__); only PR-body prose miscounts found (21 files not 22, 24 module-scope sites not 20), no code defects.

@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE @ 8ef38142d — rename verified byte-identical pre/post on every metric checked; only PR-body prose miscounts, no code defects.

Claim Verdict My evidence
82 sites, EngineBase 8, split 53/24/5, 28 shipped/54 deferred Confirmed exactly independent AST script vs. main and PR head
54 deferred = all under PENDING_ALIAS_PATHS, nothing shipped inside them Confirmed cross-checked file-by-file
Class objects identical pre/post Confirmed Engine is EngineBase→True on main; __mro__/__bases__/__module__ byte-identical for 13 classes across 4 families; __proto__ len 38, descendants(Public)=0, both sides
String-literal/cast()/TypeVar(bound=)/# type: sweep Clean no mangled literals; files with "...Datagram Protocol" strings (const/reg/, vendor/) untouched by this diff
__all__ safety ("not breaking") Confirmed, stronger 0 of all 82 alias sites (not just 14/20) put the public name in __all__
__init_subclass__ tautology Agree registering hook lives on the public class at all 5 cited lines; *Base's own hook rejects kwargs via TypeError — read the code, not just the claim
#682 dissolved Confirmed 0 RegistryWarning on import; all 3 HTTP classes fail issubclass(_, Protocol)
mypy 112 baseline Confirmed identical 112/112, same messages
pylint profile Mostly confirmed 28 other categories byte-identical; cyclic-import is non-deterministic here (121/125/142 across 3 runs, 2 on unchanged main) — noise, not a regression
Tests Pass new file 7/7 (21 subtests); test_ipv4_unit.py 26/26, all 5 cited lines present; revert reproduces both failure messages
Merge gate Clean --no-ff into 9813aa377, 0 conflicts, diff identical; reduced selection (33 tests) green
Single-PR decision Agree class-identity proof means no half-migrated hazard existed to split against

Found wrong in the PR body (not in code): "28 sites in 22 files" → I measure 21 files; "20 such modules" checked for __all__ → I measure 24 module-scope sites (14 shipped+10 pending); conclusion holds either way.

Unreproduced (environment, not code): coverage 45%→48% claim — I got 59%→63% (fixtures unbuilt, pypcap/pypcapfile absent here), same direction, different baseline; full selection showed 1 failed/27 skipped/1049 subtests vs. claimed 0/11/2546 — the 1 failure is test_tcp_runtime.py's fixture-path gap, untouched by this diff, reproduces identically on main; subtest gap is the known pytest-subtests undercount.

✅ GOOD TO MERGE @ 8ef38142d.

@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 Sep 24, 2026
@JarryShaw
JarryShaw merged commit 5c0df92 into main Sep 24, 2026
24 checks passed
@JarryShaw
JarryShaw deleted the fix/514c-alias-rename branch September 24, 2026 18:01
JarryShaw added a commit that referenced this pull request Sep 24, 2026
…st 54 sites (#752)

Completes #514 part (c). #750 renamed 28 of 82 `ProtocolBase as Protocol`
alias imports and deferred the remaining 54 -- all `ProtocolBase` -- to paths
#726 and #742 owned. Both have merged, so the deferral is over.

* renamed the alias import at all 54 sites (51 under pcapkit/protocols/, 3
  under pcapkit/foundation/) and every in-file reference that used the local
  alias: class headers, annotations, cast(), isinstance/issubclass checks,
  # type: comments, and the bracketed part of a handful of Sphinx #: doc
  comments -- 191 lines changed, no statements added
* merged two now-unaliased same-module imports per isort in 4 files
  (application.py, internet.py, link.py, transport.py), removing 4 statements
* left descriptive prose, protocol-name string literals, error-message text,
  and fully-qualified :class:/:meth:/:rtype: cross-references to the real
  public Protocol class untouched, matching #750's own precedent
* emptied tests/test_base_class_contract.py's PENDING_ALIAS_PATHS, its
  documented end state, and updated the stale docstring narrative

No behaviour change: __mro__/__bases__/__module__ identical across 18 classes
spanning every family, __proto__ registry 38 keys before and after,
descendants(Protocol) 0 in both. mypy stays at the 112-error baseline.
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 24, 2026
JarryShaw added a commit that referenced this pull request Sep 25, 2026
…ote (#763)

- `Extractor.register_engine`'s `# NOTE:` explained its
  `issubclass(engine, EngineBase)` gate by citing `engines/pcap.py` importing
  `EngineBase as Engine`. #750 removed that alias: the file now imports
  `EngineBase` under its own name and declares `class PCAP(EngineBase[Frame])`,
  so the parenthetical pointed a reader at a shape that is not there.
- Cites the class statement rather than a line number, a line citation being the
  same staleness class. The conclusion is unchanged and still correct: the
  built-ins subclass the base directly to decline `Engine.__init_subclass__`'s
  auto-registration, which is why the gate is wide. `See #513.` kept.

Comment-only. Verified no `as Engine` survives anywhere under `pcapkit/`, and that
the file's AST is identical to `origin/main`'s (434 statements both sides), so
coverage is unchanged. tests/foundation 259 passed / 11 skipped / 404 subtests
passed, plus tests/test_base_class_contract.py.

Fixes #756.
@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

refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant