fix(protocols): widen the two kept get overrides in mh.py to accept default

[JarryShaw]

• Searched for similar pull requests
• Followed the coding style (make pylint, make mypy, make isort)
• make test passes, and a test case covers the change
• Added a changelog entry -- N/A -- changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

• fix — corrects a defect

---

Description of your pull request and other information

Fixes #935. FastBindingAcknowledgmentStatus.get and IPv6AddressPrefixCode.get were re-parented onto EnumLookup by #932, which made them advertise the base's two-argument get(key, default) through inheritance while their own kept @staticmethod overrides still only accepted one — default= raised TypeError instead of the base's own fallback.

Per the owner's ruling on #935 ("I lean on 1"), both signatures now widen to get(key, default=NO_DEFAULT) and resolve default through _value2member_map_, matching the base's contract exactly — deliberately not through the constructor already used for key, since that constructor's own miss path (_missing_) raises loud and would pollute sys.tracebacklimit process-wide even on a miss this method answers quietly (the #362 hazard). This removes the two # type: ignore[override] # pylint: disable=arguments-differ suppressions the old signature mismatch needed; a narrow # type: ignore[misc] covers the four _value2member_map_ reads instead, since that attribute is generic over Self and reading it through a bare class reference (no cls — the @staticmethod is kept) is what mypy flags ambiguous.

Verified against 382375811: mypy (321 errors/38 files) and pylint (8.67/10, exit 30) are unchanged, with the two former [override] errors now gone; isort clean. New tests in both owned test files prove the TypeError is fixed, default is honoured exactly like the five pure re-parents, and the quiet raise (#933) still holds on the widened path.

[JarryShaw]

NEEDS CHANGES at 2bdf69b64 — Opus cross-review (author was Sonnet). One blocking item, re-derived by me.

tests/corekit/test_enum_lookup_reparent_930_unit.py:28-30 carries the rejected implementation's rationale. It says default is "resolved by hand (through the same constructor the override already calls for key, since :meth:_missing_ never mints on either path)". The shipped code does the opposite, and mh.py:700-707 in the same commit says so outright: "not the constructor this method already calls for key below: on a miss, that constructor reaches :meth:_missing_, which raises loud". Both the mechanism and the reason are wrong — and this is the file the next reader will open to find the design decision.

Confirmed otherwise, and the verification was thorough enough to be worth naming: a 10-case × 4-class matrix (name hit/miss ± usable and unusable default, default-as-name, NO_DEFAULT, value hit/miss ± default) found all four classes identical in result, sys.tracebacklimit and log count — so the two overrides really are call-for-call with the pure re-parents. The cross-class map trap was tested with values present in only one map (FBAS {0,128,129,130,131}, IPC {2,3,4}): each reads its own. All 10 members round-trip by name and value, 1-arg and 2-arg. mypy 321/38 on both trees, diff only two pre-existing [arg-type] lines shifted; no [override], no unused-ignore, and mypy.ini:8 sets warn_unused_ignores = True so an unnecessary ignore would already surface. Coverage: mh.py's missing-line list starts at 1063, nothing in 660–1000, so every new line runs.

The module-purge alibi holds, tested rather than assumed: running test_mh_unit before the enum module fails on the pristine base tree too — 15 failures / 4 errors, same EnumKeyError escaping assertRaises. Pre-existing, not this PR.

Two pre-existing findings, out of scope here — I will track them rather than widen this PR:

1. A successful 2-argument int-miss pollutes the process. FastBindingAcknowledgmentStatus.get(9999, 0) returns a member yet leaves sys.tracebacklimit = 0 and one CRITICAL log record, because resolving key reaches the loud _missing_. Measured identically on the pure re-parent LMAAddressCode, so it is convergence and not a divergence — but this PR closes that hazard for default and documents it at length while the same hazard stays live on key, on the exact surface it opens.
2. Non-int/non-str keys still diverge. get(None) and get(1.5) give a quiet EnumKeyError on the two overrides and a loud EnumValueError on the pure re-parents, because the overrides branch on isinstance(key, int) and fall through to the name path, where the base branches on isinstance(key, str). So "all seven behave alike" is not yet literally true.

Also worth fixing while in there: the new test_mh_unit.py test leaves sys.tracebacklimit = 0 set for the rest of the process without restoring it.

[JarryShaw]

GOOD TO GO at c230bb5d0.

The delta from the reviewed head 2bdf69b64 is test files only — pcapkit/ untouched — so nothing the Opus review confirmed at that head is disturbed. I verified both changes myself rather than re-reviewing:

The docstring now states both halves correctly, tests/corekit/test_enum_lookup_reparent_930_unit.py:27-36: default is "resolved by hand through _value2member_map_ -- the base's own route, not the constructor the override already calls for key -- because that constructor's miss path, :meth:_missing_, raises loud", with the sys.tracebacklimit pollution and #362 named. Mechanism and reason both right, and consistent with mh.py:700-707 in the same commit.

The tracebacklimit leak is closed, with an addCleanup registered before any call that can set it. Measured: after running the test, hasattr(sys, 'tracebacklimit') is False. Both target tests pass — test_mh_get_default_widens_the_kept_overrides OK in 0.660s, KeptOverrideQuietnessTests 3/3 OK.

Standing on the Opus review at 2bdf69b64, whose verification was the most thorough of this batch: a 10-case × 4-class matrix found both overrides identical to the pure re-parents LMAAddressCode and LocalizedRoutingStatus in result, sys.tracebacklimit and log count; the cross-class map trap was tested with values present in only one map and each class reads its own; all 10 members round-trip by name and value in both call shapes; mypy 321/38 on both trees with no [override] and no unused-ignore, and mypy.ini:8 sets warn_unused_ignores = True so a redundant ignore would already surface; coverage shows every new line executed. The module-purge alibi was tested, not assumed — the same 15 failures / 4 errors reproduce on the pristine base tree.

Two follow-ups I am tracking rather than folding in, both pre-existing and measured on the pure re-parents too, so neither is a divergence this PR introduces:

1. A successful 2-argument int-miss still pollutes the process — get(9999, 0) returns a member yet leaves sys.tracebacklimit = 0 and one CRITICAL record, because resolving key reaches the loud _missing_. This PR closes that hazard for default and documents it at length while it stays live on key, on the same call.
2. Non-int/non-str keys still diverge: quiet EnumKeyError on these two overrides, loud EnumValueError on the pure re-parents. So "all seven behave alike" is not yet literally true.

Unpublished and awaiting you. #939 also edits tests/protocols/internet/test_mh_unit.py — three lines — so whichever of the two lands second needs a rebase. That is a consequence of my partition error, not of either PR.

[JarryShaw]

why must we have the two overrides tho? cant they directly fall back to the base class's?

[JarryShaw]

New question recorded, verbatim:

why must we have the two overrides tho? cant they directly fall back to the base class's?

Measured, and the answer is: they can — after this PR they are behaviourally redundant. Comparing cls.get(...) against the base implementation called directly (EnumLookup.get.__func__(cls, ...)) over nine cases per class — name hit, name miss, value hit, value miss, and every default combination:

FastBindingAcknowledgmentStatus: 9 cases, 0 differ between override and base
IPv6AddressPrefixCode:           9 cases, 0 differ between override and base

There is exactly one input where they differ, and the override is the worse of the two. Non-int/non-str keys:

override  get(None): EnumKeyError    tb='unset' log=0
base      get(None): EnumValueError  tb=0       log=1

The override branches on isinstance(key, int) and falls through to the name path for anything else; the base branches on isinstance(key, str) and treats the rest as a value. So get(None) gets a KeyError-shaped exception on these two classes and a ValueError-shaped one on the other five. That is the divergence the cross-review flagged, and deleting the overrides removes it rather than needing a separate fix.

Nothing else is holding them up. No aliases exist on either — __members__ 6 and 4, list(cls) 6 and 4, zero aliases — so the "Backport support for original codes" the docstrings cite is just the int-or-name dual resolution, which the base already does. They predate EnumLookup entirely: they came in with the FMIPv6 work (5ba6ff05f, #383), were carried through the registry completion (489eef651, #437), and were last touched to stop minting on unassigned bytes (fde4cb20c, #889) — a job EnumLookup now does for all seven.

So the honest answer is that #935 solved the wrong problem: widening the signatures makes them faithful copies of the base rather than removing the duplication, and I should have asked your question before dispatching the work.

Two options, and I would take the second:

1. Merge fix(protocols): widen the two kept get overrides in mh.py to accept default #940 as it stands, then delete the overrides in a follow-up. Safe, but it lands 187 lines of mh.py that exist only to be removed.
2. Delete both overrides in fix(protocols): widen the two kept get overrides in mh.py to accept default #940 instead. The signature-widening and the two [override] suppressions both disappear, get(None) converges with the other five classes, and fix(mh): two kept get overrides advertise the base default argument and reject it with TypeError #935 closes properly. Smaller diff, and it makes the "all seven behave alike" claim literally true — which it still is not today.

The one thing to confirm before (2): whether anything calls these with a non-int/non-str key and relies on the KeyError shape. EnumKeyError derives from both KeyError and BaseError, EnumValueError from ValueError, so a caller catching KeyError would start missing. I will grep for that either way and report before touching anything.

[JarryShaw]

I prefer (2) directly.

[JarryShaw]

Ruling recorded, verbatim:

I prefer (2) directly.

So option 2: delete both get overrides in pcapkit/protocols/internet/mh.py rather than widening them. FastBindingAcknowledgmentStatus and IPv6AddressPrefixCode inherit EnumLookup.get outright.

What that buys, all measured before you ruled: the two # type: ignore[override] # pylint: disable=arguments-differ suppressions go with them; get(None) and get(1.5) stop diverging from the other five classes — quiet EnumKeyError becomes loud EnumValueError, matching the base — so "all seven behave alike" becomes literally true, which it is not today; and ~187 lines of mh.py that exist only to reimplement the base come out.

The evidence it is safe: zero behavioural difference from the base across nine cases per class (name hit/miss, value hit/miss, every default combination); zero aliases on either (__members__ 6 and 4, list(cls) 6 and 4), so the "Backport support for original codes" the docstrings cite is just int-or-name dual resolution the base already does; and all 20 call sites are in tests, none in pcapkit/, none passing a non-int/non-str key — so nothing depends on the KeyError shape that deletion changes.

Dispatching the rewrite now. The PR keeps its number and branch; the diff inverts from +321/-62 to a deletion, and the two tests added for the widening get repointed at the inherited behaviour instead — including one that pins get(None) raising EnumValueError like the other five, since that convergence is the point.

needs: decision removed, wip applied. My own note on this: #935 solved the wrong problem and I should have asked your question before dispatching it. Widening made the overrides faithful copies of the base instead of removing the duplication — the redundancy was visible in the diff I reviewed and I did not see it.

[JarryShaw]

GOOD TO GO at 4254183d2 — the deletion is done, and I verified the force-push incident below did not lose anything.

Per your ruling "I prefer (2) directly.", both get overrides are gone rather than widened:

• def get( count in mh.py: 0. On main there were two, at :645 and :782, each carrying # type: ignore[override] # pylint: disable=arguments-differ. Both methods and both suppressions are gone. The one surviving type: ignore[override] is on _make_data, pre-existing on main at :1791 and unrelated.
• mh.py is a net −50 lines (110 insertions, 160 deletions); both classes now report 'get' not in vars(cls) and inherit the base classmethod.
• NonCanonicalKeyConvergenceTests.test_none_and_float_keys_all_raise_enumvalueerror is new and pins the point of the ruling: get(None) and get(1.5) now raise EnumValueError on all seven. Proven to fail against c230bb5d0 — the widened version — with 4 errors, EnumKeyError instead of EnumValueError on both classes. "All seven behave alike" is now literally true.
• The three tests that described the old shape were re-aimed rather than deleted: ReparentedBasesTests drops its staticmethod assertions (nothing left to decorate), KeptOverrideQuietnessTests → InheritedQuietnessTests, and PureReparentClassmethodTests → AllSevenInheritTheBareClassmethodTests, extended from five classes to all seven. The tracebacklimit addCleanup is retained and re-verified.
• mypy 321/38 unchanged with no [override] and no unused-ignore; pylint 8.67, exit 30, the only mh.py delta being two fewer C0209 consider-using-f-string — both from the deleted raise EnumKeyError('%r is not a valid %s' % …) calls, so a consequence of the removal rather than a new finding.

The incident, reported by the worker and independently checked by me. A merge of main into the branch (reconciling #939) was discarded by a force-push, because --force-with-lease compared against a stale local tracking ref — it protects against a remote you have fetched, and is no protection at all if you have not. The worker caught it from the push output's "before" sha, fetched, and rebased its single commit onto 83c7552b8.

I did not take "nothing was lost" on trust. git merge-base --is-ancestor origin/main 4254183d2 returns true, so every commit on main is contained in this branch; #939's rename survives at test_mh_unit.py:1959, :1965, :1969; one commit, MERGEABLE.

Unpublished and awaiting you. #935 solved the wrong problem and your question caught it — the redundancy was visible in the diff I had already granted good-to-go, and I should have asked before dispatching the widening.