Skip to content

refactor(protocols): import ProtocolBase under its own name in the last 54 sites - #752

Merged
JarryShaw merged 1 commit into
mainfrom
refactor/514-protocolbase-alias-part-c
Sep 24, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
refactor/514-protocolbase-alias-part-c

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • refactor -- changes neither behaviour nor performance

Description

Fixes #514.

#750 renamed 28 of the 82 ProtocolBase as Protocol alias imports and deferred
the remaining 54 (all ProtocolBase, under pcapkit/protocols/ and three
pcapkit/foundation/ modules) to paths #726 and #742 owned. Both have merged,
so this completes the rename.

Renamed the alias import at all 54 sites plus 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. 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. Four files
(application.py, internet.py, link.py, transport.py) needed their two
now-unaliased same-module imports merged per isort. Emptied
tests/test_base_class_contract.py's PENDING_ALIAS_PATHS -- its documented
end state -- and updated the now-stale docstring narrative.

Verification:

  • 191 lines changed, no statements added (4 removed from the isort merges)
  • mypy: 112 errors in 38 files, byte-identical to origin/main before and after
  • Class-identity proof: __mro__/__bases__/__module__ identical across 18
    classes spanning every touched family; pcapkit.protocols.__proto__ 38 keys
    before and after; descendants(Protocol) 0 in both
  • Falsification: test_base_class_contract.py's two alias tests fail on
    origin/main with PENDING_ALIAS_PATHS emptied, pass with this rename
    (python -m unittest, and coverage run -m pytest, not pytest-cov)
  • Ran (not make test, which is forbidden here): tests/test_base_class_contract.py,
    tests/protocols/ (by subdirectory), tests/foundation/registry/test_protocols.py,
    tests/foundation/test_extraction*.py, tests/foundation/traceflow/ --
    746 tests / 2995 subtests, all passing, run in memory-bounded batches

No breaking label: the produced class objects are identical to before (see
class-identity proof above), so no correct caller code changes behaviour.

…st 54 sites

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 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 @ 829314ef4

Independent cross-review on a different model (opus); first review on this PR. Every claim re-derived myself in a worktree pair, 73f09aecb vs 829314ef4.

Claim Evidence I obtained myself
54 sites = 51 protocols/ + 3 foundation/ ✅ AST census, not grep: 54 → 0; all 71 ProtocolBase imports now unaliased. FieldBase as Field (10) correctly left — NOT_IN_SCOPE excludes it by design
PENDING_ALIAS_PATHS empty ✅ {}. is_pending() is now always False, so all 82 sites are checked — stricter, not vacuous; and startswith(()) is False, so the runtime sweep skips nothing
191 lines, 4 statements removed ✅ --numstat over pcapkit/: 54 files, 191+/195−, net −4 = the 4 isort merges. _PT, _ST, ProtocolBase intact in all four
mypy 112 in 38, byte-identical ⚠️ Same 112/38; error text, codes and columns identical. But not byte-identical: 7 line numbers in internet.py shift −1, from that file's own isort merge. Substance right, wording overstated
Class identity ✅ Fingerprinted all 44 ProtocolBase descendants, not 18: __mro__/__bases__/__module__/issubclass/abstractness byte-identical; __proto__ 38 keys identical; descendants(Protocol) 0 on both; 327 modules import on both
Falsification ✅ Both alias tests fail on main with the dict emptied (2 failed under unittest and coverage run -m pytest), pass at head (2 passed, both runners)
Nothing wrongly rewritten ✅ 0 Literal[ lines and 0 RegistryError lines changed anywhere in the diff. All 8 'must be a Protocol subclass' messages sit intact beside issubclass(…, ProtocolBase) — #750's precedent matched exactly. Only 7 #: lines changed, bracketed type only, trailing prose left as Protocol
Nothing missed ✅ 0 unqualified :class:Protocol``; 0 # type: comments still naming `Protocol` (5 existed on `main`); `get_type_hints()` over all 54 modules — 1047 resolved / 1329 pre-existing failures, byte-identical sets, none naming `Protocol`
746 tests / 2995 subtests ⚠️ Exact, but the selection is narrower than described — see below

The test gap, and it is closed. "tests/protocols/ (by subdirectory)" ran the six subdirectories and omitted the 13 top-level test_*.py files directly under tests/protocols/: 867 − 121 = 746 and 3565 − 570 = 2995, to the digit. Those omitted files include test_protocol_base_unit.py, test_dispatch_*_unit.py and test_protocol_code_registration_unit.py — among the most relevant tests here. I ran them: 121 passed / 570 subtests, and ran the whole fixture-dependent set on both trees, getting byte-identical results (754 passed / 3413 subtests each, 0 failures). Full stated selection plus the omitted slice = 867 tests, all green.

Widened scope (cast() targets, # type: comments, @overload signatures) was necessary, not overreach: those are positions mypy resolves, and the unchanged 112 proves the renames were required and complete. No TypeVar(bound=) sites existed here; # type: ignore[...] codes untouched, so no suppression churn. Every renamed name reference is semantics-preserving by construction, since inside those 54 modules Protocol provably was ProtocolBase. Worth stressing Protocol is a genuine distinct subclass (protocol.py:1880), not an alias — so class IPv4(Protocol) really did say the wrong thing, making this a correctness-of-source fix, not a cosmetic one.

breaking correctly absent, with a caveat the description should carry: 10 of the 54 were runtime imports rather than TYPE_CHECKING, so 10 modules stop exposing a Protocol attribute (41 → 31). No correct caller relied on it — 0 modules list it in __all__, ProtocolBase is documented "internal use only", and the new runtime test now enforces the absence — but it is an observable change that "class objects are identical" does not cover.

Also, not blocking: line-too-long 369 → 374 — five lines newly cross the repo's own 120 limit by 2–3 chars (registry/protocols.py ×2, misc/pcap/frame.py, misc/pcapng.py, schema/internet/ipv6_route.py), and ~14 already-over lines grew 4–8 chars, arithmetic from a 4-char-longer name on ModuleDescriptor[…] | Type[…] pairs. Total pylint messages fall 6053 → 6040 (cyclic-import −19), and make pylint already exits 30 on main, so nothing green regressed; the author already used # pylint: disable=line-too-long on one such line and extending that would settle it. Template followed (4-item checklist, one type ticked, both standing exceptions worded correctly; only the 7 unticked type boxes were pruned). Merge-tested: origin/main is already an ancestor, --no-ff → "Already up to date", tree sha unchanged — the tree I tested is the merged tree. Unrelated to this PR: make isort fails identically on both trees at pcapkit/protocols/schema/schema.py, i.e. already red on main.

✅ GOOD TO MERGE @ 829314ef4 — good for main @ 73f09aecb

@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE @ 829314ef4 — 54→0 alias sites (AST-verified), all 44 ProtocolBase descendants byte-identical, mypy 112/38 unchanged, falsification reproduced; caveats: "byte-identical" mypy wording, +5 line-too-long, and 121 tests the author's selection skipped (I ran them, green).

@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

Copy link
Copy Markdown
Owner Author

We explicitly stated in the design that we would deliberately like to check for Protocol subclasses when it's a custom non-builtin class (which are all ProtocolBase subclasses).

@JarryShaw

Copy link
Copy Markdown
Owner Author

That design intent is not implemented today, and was not before this PR either — the alias was hiding
it. Measured on both trees:

sites reading issubclass(protocol, Protocol) what they actually check
main @ 73f09aecb 6 ProtocolBase — every one of those files imports ProtocolBase as Protocol
this PR @ 829314ef4 0 ProtocolBase, written as ProtocolBase

internet.py:164, link.py:144, transport.py:115, frame.py:151, misc/pcapng.py:886 and
registry/protocols.py:216 all read as Protocol checks and are all base checks. Nothing in pcapkit/
checks the real public Protocol on either tree
, so this PR changes no behaviour — it makes the gap
legible instead of creating it. That is the argument for merging it as-is.

The gap itself is real and worth its own change: if a custom class should be required to subclass
Protocol to register, while built-ins reach the registry as ProtocolBase subclasses, then the check has
to distinguish them — one predicate cannot, since every Protocol subclass is also a ProtocolBase
subclass. Note #513 widened these checks in the first place precisely because the narrow form rejected
pcapkit's own built-ins
, so re-narrowing needs a second criterion rather than a revert.

Worth recording, because it strengthens the case for the rename: Protocol is ProtocolBase is False —
Protocol is a genuine subclass at protocol.py:1880, MRO Protocol → ProtocolBase → Generic. I had
previously said they were the same object and was wrong. So class IPv4(Protocol) really did name a
different class than it inherited; this was never cosmetic.

Shall I file the custom-versus-builtin check as its own issue? It is #514's design intent, it is
unimplemented, and it needs a criterion that does not exist yet — not something to bolt onto a rename.

@JarryShaw
JarryShaw merged commit 7acf56e into main Sep 24, 2026
24 checks passed
@JarryShaw
JarryShaw deleted the refactor/514-protocolbase-alias-part-c branch September 24, 2026 22:21
@JarryShaw

Copy link
Copy Markdown
Owner Author

Recorded: no issue filed — the custom-versus-builtin check is not wanted. So the widened
issubclass(protocol, ProtocolBase) is the intended behaviour, not a gap awaiting a narrower criterion,
and #513's widening stands as the final answer rather than a stopgap.

That leaves this PR purely a naming correction with nothing deferred behind it: 6 sites read
issubclass(protocol, Protocol) on main while checking ProtocolBase through an alias, and 0 read that
way here. Nothing is left open on #514 by this decision beyond the PR itself.

Worth leaving on the record for the next reader of those six sites, since it is the question that will
occur to them too: the check cannot distinguish a custom class from a built-in with one predicate, because
every Protocol subclass is also a ProtocolBase subclass — and the narrow form was tried and rejected
pcapkit's own built-ins
, which is why #513 widened it. The error text still reads "must be a Protocol
subclass" beside a ProtocolBase check at all 8 message sites; that wording is now the only remaining
trace of the distinction, and it is arguably worth aligning to the check some day. Not a blocker, and not
something I am filing.

#752 remains ✅ review: good-to-go at 829314ef4, merge-verified against main @ 73f09aecb —
origin/main is already an ancestor, so the reviewed tree is the merged tree.

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

None yet

Development

Successfully merging this pull request may close these issues.

Design: adopt the EnumMeta/EnumSchema opt-in registration pattern for Protocol, Engine, Reassembly and TraceFlow

1 participant