Repository navigation
fix(mh): decide whether the two kept get overrides should raise quietly like the base #933
Description
FastBindingAcknowledgmentStatus.get and IPv6AddressPrefixCode.get (both in pcapkit/protocols/internet/mh.py) raise EnumKeyError on a name miss without quiet=True. The base does the opposite — EnumLookup.get raises with quiet=True at pcapkit/corekit/enum.py:412.
That divergence pre-dates #932 and is untouched by it, but #932 makes it newly reachable: re-parenting those two onto EnumLookup gives them an inherited get_all, which did not exist on them before and which routes a miss through the loud override. Measured on both trees:
post-#932 has get_all=True raised=EnumKeyError sys.tracebacklimit '<unset>' -> 0
pre-#932 has get_all=False raised=- sys.tracebacklimit '<unset>' -> '<unset>'
A loud BaseError sets sys.tracebacklimit = 0 process-wide — the hazard behind #362 — so this is a public-API path that can now silently truncate every later traceback in the process. No non-test caller exists in pcapkit/ today, which is why #932 is not being held for it.
The decision I do not want to take inside a refactor: should these two overrides adopt the base's quiet=True?
- Yes — they are the odd ones out, and the fix(corekit,utilities): raise pcapkit exceptions from EnumLookup.get, following stdlib Enum's shape #923 ruling on failed-lookup shape reads as a house-wide contract rather than a per-class choice.
- No — a
get_allmiss is a genuine failure the caller should see loudly, andquiet=Trueon the base exists specifically because a name miss there is part of a successful call atMethod.get. These overrides have no such caller, so the reasoning does not transfer.
Option 2 has the better argument on the code as it stands, so unless you say otherwise I would keep them loud, pin the behaviour with a test, and document why they differ. #932 already does the pinning and documenting, so this issue only decides whether the raise itself changes.
Surfaced by the Opus cross-review of #932 and verified independently.
Activity
JarryShaw commentedon Sep 29, 2026
No.
JarryShaw commentedon Sep 29, 2026
Ruling recorded, verbatim:
So the two
@staticmethod getoverrides inpcapkit/protocols/internet/mh.py(FastBindingAcknowledgmentStatusat:644,IPv6AddressPrefixCodeat:763) keep raising loudly and do not adopt the base'squiet=True. That is option 2, and it is the reading with the better argument on the code as it stands:quiet=Trueatpcapkit/corekit/enum.py:412exists because a name miss there is part of a successful call atMethod.get, and these two overrides have no such caller.No code change follows from this. What does follow is the recording, which #932 already carries as briefed before the ruling landed: a test pinning the current loud behaviour of the inherited
get_allon both classes, and a line in each override's docstring saying why it differs from the base. I will verify both are actually present before #932 gets a good-to-go rather than taking the report for it.Staying open as
wipuntil #932 merges with that pin in place, so the decision is not recorded in a closed issue while the thing implementing it is still in flight.JarryShaw commentedon Sep 29, 2026
Oh wait. I meant, they should follow house convention and not to be loud.
JarryShaw commentedon Sep 29, 2026
Reversed, and recorded verbatim:
So the answer is option 1, not option 2. Both
@staticmethod getoverrides inpcapkit/protocols/internet/mh.py—FastBindingAcknowledgmentStatusat:644andIPv6AddressPrefixCodeat:808— adopt the base'squiet=Trueand stop raising loudly. My previous comment recorded the opposite; this supersedes it.That makes this a code change rather than a documentation one, and it reverses three things #932 had already landed on the strength of the earlier answer: the
quiet=Truemust be added to both raises,KeptOverrideLoudnessTestsmust assert quiet instead of pinning loud, and both override docstrings must stop citing #933 as a ruling to stay loud. #932's worker is applying that now.Worth stating plainly since it argues against the change: the reason I recommended option 2 was that
quiet=Trueatpcapkit/corekit/enum.py:412exists for a name miss inside a successful call atMethod.get, and these overrides have no such caller. The house-convention argument beats it — aBaseErrorthat setssys.tracebacklimit = 0process-wide is a cost the whole library pays, so uniformity is worth more than a per-class judgement about callers. Recording the reasoning I got wrong rather than quietly switching sides.This also closes the hazard #932 surfaced: the newly-inherited
get_allreached the loud raise, which is what prompted this issue.JarryShaw commentedon Sep 30, 2026
Closing: the pin landed and I verified it on
mainrather than taking the report.#932 merged as
382375811. Onmainnow:quiet=Trueon both raises —pcapkit/protocols/internet/mh.py:697(FastBindingAcknowledgmentStatus.get) and:834(IPv6AddressPrefixCode.get), matching the base's shape atpcapkit/corekit/enum.py:412.KeptOverrideQuietnessTestsattests/corekit/test_enum_lookup_reparent_930_unit.py:417, pinning that bothgetand the newly-inheritedget_allstay quiet on both classes.The reversal recorded here is therefore implemented, not just decided. Worth restating why, since the reasoning is the durable part: the base raises with
quiet=Truebecause a name miss there sits inside a successful call atMethod.get, and a loudBaseErrorsetssys.tracebacklimit = 0process-wide (#362). Your ruling extended that to these two overrides on house-convention grounds, which beats the per-class argument I had made for keeping them loud — uniformity is worth more than a judgement about current callers, because the process-wide cost is paid by the whole library.docs(contributing): record the get-override rulings on the convention…
docs(contributing): record the get-override rulings on the convention…
docs(tests): paraphrase the six rulings this file still quoted
docs(tests): paraphrase the six rulings this file still quoted
docs(tests): say what the #933 answer was, not how it was read
docs(contributing): refer to the owner by role, not by gendered pronoun