Skip to content

fix(protocols): parser registrars warn only when the entry really changes (#1362) - #1400

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1362-parser-registrar-identity
Oct 8, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1362-parser-registrar-identity

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style — isort clean; pylint/mypy (Makefile flags) on the 9 sources: no new mypy errors, pylint adds 3 R0801 duplicate-code (the shared guard). make targets not run.
  • make test passes, and a test case covers the change — the legs below only
  • Changelog entry — N/A, added centrally

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1362. Refs #1363.

The 14 parser registrars (IPv4, HOPOPT, IPv6-Opts, IPv6-Route, HIP, MH ×3, TCP ×2, SCTP ×3, HTTP/2) now warn only when the incumbent differs from the replacement; an equal entry is a silent no-op and keeps the stored object. PCAP-NG's four (register_block/_option/_record/_secrets) are left for the PCAP-NG work in #1398. The traceflow.py dumper guard already compares identity after #1376 and is unchanged.

Note on #1363: these registrars refuse nothing, so the shipped entry is already accepted back; the ruling here means restoring it after an override warns once (a real displacement) and re-restoring is then silent. No snapshot is needed, unlike the class registrars in #1376.

Probe: re-registering every shipped entry warned 14/14 sites (271 entries) on origin/main, 0 now.

Tests: new tests/protocols/test_parser_registrar_identity_unit.py, 5 tests × 14 sites. With origin/main sources: 42 subtests fail; the foreign-warns and unregistered-silent guards pass both ways. Eleven existing "warns on overwrite" tests re-registered the shipped value and now overwrite with a different one. run_unittest_leg.py protocols/internet: 552, 0 failures. protocols --exclude protocols/internet: 1009, 0 failures. pytest tests/foundation/traceflow/: 25 passed. pytest tests/project: 399 passed, 1 skipped.

…nges (#1362)

- The 14 registrars that store a parser (IPv4, HOPOPT, IPv6-Opts,
  IPv6-Route, HIP, MH x3, TCP x2, SCTP x3, HTTP/2) tested presence only,
  so handing back the stored entry warned of an overwrite. They now
  compare the incumbent with the replacement: an equal entry is a silent
  no-op and the stored object is kept.
- Under #1363 the shipped entry is accepted back after an override, which
  undoes it; that restore warns, as any real displacement does.
- Eleven existing "warns on overwrite" tests re-registered the shipped
  value; they now overwrite with a different one.

New tests/protocols/test_parser_registrar_identity_unit.py: 5 tests over
14 sites; 42 subtests fail on origin/main.
@JarryShaw JarryShaw added the fix Pull requests that fix a defect (fix: subject prefix) label Oct 8, 2026
@JarryShaw JarryShaw added the review: running A cross-review is in flight against the current head - no verdict yet label Oct 8, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 5cbed8ebf: GOOD TO GO (Sonnet reviewed; author Opus).

  • == is safe.
    • Stored values are only str and tuples of callables. A swapped (g, f) against a stored (f, g) still warns, and distinct lambdas never compare equal.
    • A falsy non-None incumbent such as '' still warns when replaced.
    • No stored type makes == raise.
  • .get changes nothing. All 14 containers are defaultdicts, and neither .get nor the old in inserts a missing key.
  • No test was weakened. All 11 modified tests still assert one warning and the new stored value on a real displacement. The IPv4 and HIP tests also gained a check that a repeat stays silent.
  • No site was missed. Of 18 parser registrars in pcapkit/protocols, 14 are guarded here. The other 4 are PCAP-NG's, now tracked in fix(pcapng): the four PCAP-NG parser registrars warn when handed their own entry #1401.
  • Revert proof: 42 failed, as claimed. The modified test modules pass, and protocols/internet gives 552/0. I re-ran the new module myself and it passes (5/0).

Nit: sibling class-storing registrars compare with is not, so == here is a deliberate widening for strings and does no harm.

@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.44% (unit tier, Python 3.14, 5cbed8ebf, 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 2018 88 628 24 94.78%
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 16369 205 4352 167 98.17%
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.

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): thirteen code-keyed registrars warn "already registered, overwriting" on re-registering the identical object

1 participant