Skip to content

fix(registry): re-registering a stored ModuleDescriptor keeps it (#1364) - #1415

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1364-registry-descriptor
Oct 8, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1364-registry-descriptor

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 8, 2026 •

Copy link
Copy Markdown
Owner

Closes #1364

ProtocolBase.register and the Internet/Link/Transport/SCTP/Frame/PCAPNG overrides now compare the argument with the stored entry before resolving a ModuleDescriptor: the stored entry, or an equal descriptor, is a silent no-op that keeps it; a different entry still warns; invalid input still raises. Extractor.register_dumper was already fixed by #1376 and is pinned only.

Probe, TCP.register(21, TCP.__proto__[21]) (same for IPv4, Ethernet, SCTP, Frame, PCAPNG): before — 1 warning, entry becomes the class; after — 0 warnings, entry is the stored descriptor.

Tests: new tests/protocols/test_registry_stored_descriptor_unit.py, 6 passed + 28 subtests; on origin/main 8 failed (SCTP alone reverted: 1). Legs: 522 / 1059 / 566 tests, 0 failures; tests/project 399 passed, 1 skipped.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 8, 2026
- ProtocolBase.register and the Internet, Link, Transport, SCTP, Frame
  and PCAPNG overrides resolved a ModuleDescriptor argument to its class
  before comparing it with the stored entry, so handing back the stored
  descriptor warned and replaced the deferred entry with the class. They
  now compare first: the stored entry, or a descriptor equal to it, is a
  silent no-op that keeps the stored object. A different entry still
  warns, and invalid input still raises RegistryError.
- Extractor.register_dumper already keeps its shipped descriptors since
  #1376; it is pinned in the new test.

New tests/protocols/test_registry_stored_descriptor_unit.py: 6 tests;
8 fail on origin/main (7 subtests and the ProtocolBase case).
@JarryShaw
JarryShaw force-pushed the fix/1364-registry-descriptor branch from cacef95 to 584d5b0 Compare October 8, 2026 13:29
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 8, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 584d5b02a. Reviewed by Sonnet; the author ran on Opus.

  • Repro: on main, all seven registrars (Internet, Link, Frame, PCAPNG, SCTP, Transport via TCP/UDP, ProtocolBase) warn and swap the stored descriptor for the class. On this head there are 0 warnings, and __proto__[code] is the original, still-unimported descriptor.
  • Semantics: an equal copy of the stored descriptor is a silent no-op that keeps the stored object. A class over a stored descriptor still warns, which matches the docstrings. A descriptor over a stored class is silent, as on main. Nothing diverges from fix(protocols): parser registrars warn only when the entry really changes (#1362) #1400.
  • Invalid input: a plain tuple equal to a stored descriptor still raises RegistryError, as do non-class inputs.
  • Coverage of registrars: the dumper, engine, reassembly and traceflow registries already keep shipped descriptors (question(registry): should a built-in engine, reassembly or traceflow overridden by name be restorable through the public registrar? #1363/fix(registry): foundation registrars restore the built-in a name shipped with (#1363) #1376). Re-registering all 18 is silent and keeps identity, and no other registrar has this pattern.
  • Tests: 8 of the new module's tests fail with the 7 source files reverted. protocols --exclude protocols/internet gives 1059 tests with 0 failures, and foundation 522 with 0 failures.
  • Nits, not blocking: the module-level wrappers such as register_transtype still call .klass, so going through them still imports the class (the stored entry stays a descriptor). The guard is also repeated in seven places, and one PCAPNG docstring line is overlong.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Coverage: 89.43% (unit tier, Python 3.14, 584d5b02a, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18791 968 2352 869 90.90%
pcapkit/corekit 2038 89 634 25 94.76%
pcapkit/dumpkit 234 2 84 2 98.74%
pcapkit/foundation 2637 98 928 48 94.78%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16518 219 4434 183 98.04%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2342 1006 157 43.25%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 199bfce into main Oct 8, 2026
40 checks passed
@JarryShaw
JarryShaw deleted the fix/1364-registry-descriptor branch October 8, 2026 17:22
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 8, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(registry): re-registering a stored ModuleDescriptor warns and replaces it with the class

1 participant