design: should the non-registry helper enums share a lookup interface, or be made strictly immutable?

[JarryShaw]

Follow-up from #860, moved here so the discussion has its own thread. 120 of 123 const enum classes inherit EnumRegistry; the three that do not are ftp/command.py::CommandType (IntFlag), ftp/command.py::ConformanceRequirement (IntEnum) and reg/apptype/apptype.py::TransportProtocol (IntEnum).

The owner's framing, verbatim:

IntFlag is expected to expand with its combination values; not a bug. what im thinking is, should we change those IntEnum based helper enums to base registry; giving them a shared interface. or we should strip those .get from helper methods and removing minting logic (if any) so that they stay immutable. and if we do decided to go with base registry, we can probably create another subclass from there with only necessary shared auxiliary methods, like get/register-only, no unregistered/unmited value return, nor _missing_/aliasing handlers.

So: (a) helpers onto EnumRegistry; (b) strip their .get and any minting so they stay immutable; (c) a slimmer base carrying only the shared auxiliaries.

Three measurements that constrain the answer. EnumRegistry is exactly seven classmethods — get, get_all, register, _extend, register_alias, register_aliases, _unregistered_member — and defines no _missing_: those hooks live in the generated const classes, so "no _missing_ handler" is not something a new base has to avoid.

get is already registry-free. Its body touches only cls._member_map_, cls._value2member_map_, cls(key) and the default sentinel; it never calls _unregistered_member and never mints. It is generic enum sugar, and strictly richer than the helpers' hand-rolled versions (name lookup and value lookup and a default).

get/register-only is self-contradictory, though. register guards a duplicate then calls _extend, and _extend is the mutation — registering is minting, deliberate rather than accidental. A base that registers is not immutable.

And a slim variant cannot be a subclass of EnumRegistry. Inherited methods cannot be removed, and overriding _unregistered_member to raise yields a class advertising the method it forbids. The coherent shape is a parent: EnumLookup carrying get/get_all, then EnumRegistry(EnumLookup) adding the mutating half.

That makes (c) viable only along the get line, where it converges with (b): if the helpers keep just a lookup, the only question left is whether that lookup is shared or duplicated. A repo-wide census — how many non-registry enums exist outside pcapkit/const/, and whether their .gets are the same function modulo names — is in flight and will decide it; a shared base earns its keep at 10 classes and not at 3.

[JarryShaw]

Clarification from the owner, verbatim:

and for my "get/register-only, no unregistered/unminted value reutnr, nor missing/aliasing hanalders", it means, the helper enum will ONLY have get and register logic; it will not return a unregistered and unminted value from _missing_ or get but rather directly raise; and it does not support aliasing (up to discussion)

That retracts my "get/register-only is self-contradictory" objection, and I was wrong to raise it. The distinction is between deliberate, caller-initiated registration and implicit minting on a lookup miss. register is the former; #775's defect class is the latter. A base with get + register that raises on a miss is internally consistent.

Two measurements now decide the shape.

1. The raise-on-miss property does not live in get — it lives in the subclass's _missing_. Measured on a real registry:

LinkType.get(999999)        -> <LinkType.Unassigned: 999999>   # unregistered member, via cls(key) -> _missing_
LinkType.get('NoSuchName')  -> KeyError: 'NoSuchMember'

get calls cls(key), which goes through whatever _missing_ the generated subclass defines. So "does not return an unregistered value" is obtained by not defining a permissive _missing_, not by slimming the base — a helper that omits it already raises today.

2. The real obstacle is case folding, and it breaks in whichever direction we choose.

TransportProtocol.get('TCP')  -> tcp        # helper: key.lower(), case-INsensitive
LinkType.get('ETHERNET')      -> ETHERNET   # base: cls._member_map_[key], case-SENSITIVE
LinkType.get('ethernet')      -> KeyError

Putting TransportProtocol on the base as-is breaks get('TCP'). Either the base gains case folding — a behaviour change across 120 registries — or the helper loses it, breaking its own callers. That is the decision I would want settled before any base is written.

Side finding while measuring, worth its own fix regardless: get raises KeyError for a name miss and ValueError for a value miss, and neither is a pcapkit.utilities.exceptions type, which the house rule requires for in-library errors.

On aliasing — you left it open. I would leave it out of the slim base: an alias is a second name for a registered value, which is only meaningful where names come from an external registry, and none of the three helpers has one.

[JarryShaw]

Scope widened by the owner, verbatim:

let's move the conversation to the GH issue (and the scope is not limited the three helper you listed, but actually every single enum defined in the library)

So this is no longer about CommandType, ConformanceRequirement and TransportProtocol — it covers every enum class defined anywhere in pcapkit/, and the discussion lives here rather than in chat from now on.

A repo-wide census is in flight, partitioned so the population is legible rather than a wall of 120 rows:

1. EnumRegistry subclasses under pcapkit/const/ — count, plus individually only the unusual ones: any that define get, register, register_alias or _missing_ themselves instead of inheriting, and any whose _missing_ departs from the standard return cls._unregistered_member(value, '<name>') shape. Those are what a base change could break.
2. EnumRegistry subclasses outside pcapkit/const/, if any exist — in full.
3. Every non-EnumRegistry enum anywhere in pcapkit/ — in full, with base list, member count, and which of get/_missing_/register/register_alias/register_aliases/_unregistered_member/extend_enum each defines or calls.

For group 3 the census also records the axis this decision actually turns on: does its _missing_ mint or raise — where "mint" includes delegating to a super()._missing_ that caches, as IntFlag does. Worth restating because it already caught me out once on this issue: __members__ does not reveal IntFlag composite caching; _value2member_map_ does.

The two open questions from the measurements above stand and are now library-wide rather than helper-specific: whether get should case-fold (the base is case-sensitive, TransportProtocol.get is not, and one of the two has to change), and whether aliasing belongs in a slim base at all. I will post the census here when it lands, and hold off recommending a shape until the counts are in — a shared base earns its keep at 10 classes and not at 3.

[JarryShaw]

Census in, and the population is 21 — not 3. I re-ran the count myself rather than relaying it; my AST scan over all of pcapkit/ reproduces the agent's numbers exactly:

EnumRegistry subclasses: 120   non-registry enums: 21   total: 141
non-registry with .get: 8      of those minting: 6

The eight .gets, and this is the finding that reframes the question — six of them mint:

class	file	.get mints?
FastBindingAcknowledgmentStatus	protocols/internet/mh.py	yes
IPv6AddressPrefixCode	protocols/internet/mh.py	yes
LocalizedRoutingStatus	protocols/internet/mh.py	yes
LMAAddressCode	protocols/internet/mh.py	yes
ProcedureCode	protocols/application/ngap.py	yes
ProtocolIE	protocols/application/ngap.py	yes
Criticality	protocols/application/ngap.py	no — raises
TransportProtocol	const/reg/apptype/apptype.py	no — raises, and case-insensitively

They are not eight one-offs and not one function: six near-identical copies of a minting lookup confined to two files (extend_enum(X, key, default=-1) then re-subscript), plus two genuine one-offs. The other 13 non-registry enums have no .get at all, and not one of the 21 defines register, register_alias, or calls _unregistered_member.

The shared interface (a)/(c) would create has no consumer. The two places that genuinely treat helper and registry enums interchangeably already share one, inherited from Enum: ProtocolBase._make_index (protocols/protocol.py, ~117 namespace= call sites) uses namespace[name], and EnumField (corekit/fields/numbers.py, 124 namespace classes) uses self._namespace(value). Neither would lose a line if all 21 gained get/register.

Two free subtractions available today: LocalizedRoutingStatus.get and LMAAddressCode.get have zero callers anywhere in the repo — I grepped, including tests. Dead API that advertises a minting path nobody uses.

Where that leaves the three options. (a) would re-open minting on 13 currently-closed enums and hand register_alias to things like FinalisedState and Completion. (c) cannot host the six copies that are the only duplication worth removing, since by your definition it forbids minting — and of the two it could host, TransportProtocol is under a standing ruling against ever extending it, so (c) ends up a public base for one willing caller. (b) is the coherent one, but it is the most expensive to land, because the minting is pinned by live assertions (test_mh_unit.py:1530, :1534 assert get('Vendor_specific', 200) == 200), not merely tolerated.

The strongest argument against (b), stated plainly: it leaves two permanent, contradictory answers to "what does .get mean on a pcapkit enum" — never mints on the 120 registries, mints on six helpers — plus the case-folding fork, in an API that presents as uniform. If one meaning for get across the package outweighs adding an abstraction for eight classes, then the answer is a slim base that permits an opt-in minting policy, and (b) is wrong.

One unmeasured lead worth a check before it matters: mh.py's shape mints with default=-1, so two different unknown keys would both be extended at value -1, and aenum treats a duplicate value as an alias request — the second key may resolve to the first key's member. Inferred from EnumRegistry.register's own docstring, not measured; I can settle it in three lines if you want it as a defect rather than a footnote.

[JarryShaw]

Correction to my own census above: the registry population is 124, not 120. I verified it at runtime by walking every module and testing the MRO rather than the base list:

carrying EnumRegistry (MRO):  124
  direct base:                120
  transitive:                   4  -> reg/apptype/{tcp,udp,sctp,dccp}.py::{TCP,UDP,SCTP,DCCP}
  outside pcapkit.const:        0

Those four inherit AppType, which mixes in the base, so they carry the full interface from a different file — which is exactly why a base-list AST scan misses them. They were counted in neither of my earlier buckets, so the honest totals are 145 enum classes: 124 carrying the registry interface, 21 not. EnumRegistry's own module docstring says 124, and it was right.

Two further facts that matter to this decision:

The registry protocol has never leaked out of the generated-constants layer — zero EnumRegistry subclasses anywhere outside pcapkit/const/, across 507 modules. That is evidence about demand, not just about current structure, and it cuts against extending the protocol to protocols/-layer helpers.

Three registries still mint directly from _missing_ via extend_enum — const/ipx/socket.py::Socket, const/mh/cga_type.py::CGAType, const/reg/ethertype.py::EtherType — kept deliberately per #775's ruling. Note two of those three are precisely what #775's pending branch converts, leaving CGAType as the standing carve-out. So a base change scoped against "no registry mints" would be wrong today and right after that lands.

One unreconciled number, flagged rather than papered over: tests/const/test_const_enum_no_mint.py:829 speaks of "every one of the 105". 105, 120 and 124 are three different populations in this tree and I have not reconciled which the test means. Anyone scoping a base change should settle that first — using the wrong one of the three is how a registry gets missed.

The group-3 mint/raise split, restated with CommandType qualified: 7 minting, 1 raising (Criticality), 13 closed with no _missing_ at all. CommandType sits on the mint side only in the table-growth sense — it delegates to IntFlag._missing_, which caches a composite in _value2member_map_ while leaving __members__ untouched; I measured that growth earlier on this thread (5 → 7), and you have already ruled it expected.

[JarryShaw]

My initial thought was to make them immutable - unless RFC/IANA says otherwise. Therefore they may subclass a bare base enum from pcapkit.corekit.enum - where EnumRegistry subclasses it for using in the other mutable ones.

Then there's a need to audit if based on RFC/IANA/IETF documentations and industry conventions, such an enum is extensible/dynamic/mutable then decide which base class it inherits from.

Also, if one enum is recorded in a IANA CSV format or any parsable-table-like data format, we should consider moving them to become generated by vendor crawlers rather than hardcoded and hand maintained.

[JarryShaw]

Recorded, verbatim:

My initial thought was to make them immutable - unless RFC/IANA says otherwise. Therefore they may subclass a bare base enum from pcapkit.corekit.enum - where EnumRegistry subclasses it for using in the other mutable ones.

Then there's a need to audit if based on RFC/IANA/IETF documentations and industry conventions, such an enum is extensible/dynamic/mutable then decide which base class it inherits from.

Also, if one enum is recorded in a IANA CSV format or any parsable-table-like data format, we should consider moving them to become generated by vendor crawlers rather than hardcoded and hand maintained.

So the shape is settled: a bare base in pcapkit/corekit/enum.py, with EnumRegistry subclassing it — parent, not sibling — and immutability as the default that an external registry has to earn. That also resolves the get/register question from earlier: lookup belongs on the bare base, mutation stays in EnumRegistry. needs: decision comes off; this becomes tracking work in three parts.

1. The bare base. get and get_all move down; register, _extend, register_alias, register_aliases, _unregistered_member stay up. Measured earlier: get's body touches only _member_map_, _value2member_map_, cls(key) and the NO_DEFAULT sentinel, so it needs nothing from the registry concept. Two open sub-questions that the base's signature has to settle rather than inherit by accident — whether get case-folds (the base is case-sensitive today, TransportProtocol.get is not, and one of the two must change), and that get currently raises KeyError for a name miss and ValueError for a value miss, neither from pcapkit.utilities.exceptions.

2. The audit, which gates everything else. 21 non-registry enums, of which 8 carry a .get and 6 of those mint. Each needs a documented verdict — extensible per RFC/IANA/IETF or a closed set — with the citation recorded, not inferred from its name. An audit is now in flight and will report per class: the governing document, whether the value space is open, which base it should take, and whether any current behaviour has to change to match. Note two of the 21 (LocalizedRoutingStatus.get, LMAAddressCode.get) have zero callers repo-wide, so for them the answer may simply be deletion.

3. The crawler question. Any enum whose source is an IANA CSV or another parsable table is a candidate to move from hand-maintained to generated. The audit will flag which of the 21 have such a source; that will be its own issue, since it is a different kind of change from the base split and should not ride along with it.

[JarryShaw]

Audit in. Per-class verdicts against the ruling, with the citation for each read from the source rather than inferred. Two corrections to my own earlier numbers first.

The population is 26, not 21. The base-list AST scan missed nested enum classes. Measured — 7 nested Flag classes in protocols/schema/application/httpv2.py, none inheriting EnumRegistry:

FrameType.Flags  0 members (deliberately memberless abstract base)
DataFrame.Flags  HeadersFrame.Flags  SettingsFrame.Flags
PushPromiseFrame.Flags  PingFrame.Flags  ContinuationFrame.Flags

The six concrete ones are closed sets fixed by the HTTP/2 frame definitions and want the same treatment as CommandType — a base preserving Flag semantics. FrameType.Flags has no value space at all (FrameType.Flags(0) raises TypeError: <flag 'Flags'> has no members, intentional and documented at httpv2.py:148-158), so it is out of scope rather than closed.

The verdicts. 12 rows are pure re-parenting with no behaviour change — FinalisedState, PacketDirection, PacketReception, WireGuardKeyLabel, ESPStatus, both Types, PDUKind, Criticality, ConformanceRequirement, Completion, plus the 6 httpv2 Flags. They already raise and carry no registry API.

Four contradictions, all in mh.py — FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LocalizedRoutingStatus, LMAAddressCode. Each mints today while its own docstring says the opposite, e.g. mh.py:576: ":rfc:5568#section-6.2.3 defines these values inline and IANA keeps no registry of them, so the enumeration lives here…" Closed registries that mint. Their get also carries a defect I have filed separately as #880 — a shared -1 sentinel that aliases the second unknown key to the first, and installs a member the class's own 0 <= value <= 255 guard rejects. Fix #880 first so the two changes stay separable; LocalizedRoutingStatus.get and LMAAddressCode.get have zero callers and can simply go.

Two correctly open: ProcedureCode and ProtocolIE — 3GPP TS 38.413, INTEGER (0..255) and (0..65535), 81 and 438 named. EnumRegistry is right for them, and they are also the strongest candidates for your crawler point — but their source is an installed Python dict (pycrate_asn1dir/NGAP.py), not an HTTP-fetchable CSV, so it does not fit the existing Vendor.LINK shape. ngap.py:169-171 says as much today. That is a design decision, not a mechanical port.

One needs your call: TLSKeyLabel (pcapng.py:273). It cites nothing, and its real source is the NSS SSLKEYLOGFILE format, whose label set grows as TLS features land — convention says open, the ruling's default says immutable, and there is no citation to settle it.

Two to leave alone: TransportProtocol and CommandType are already settled by your own rulings quoted in-source (apptype.py:105, and pinned at test_const_enum_no_mint.py:1959) and already behave as the ruling wants. TransportProtocol is referenced by 15 test files and 29 source files — the widest blast radius in the population — so re-parenting it is cosmetic and expensive.

Step 0 is that the bare base does not exist yet: pcapkit.corekit.enum.__all__ is ['NO_DEFAULT', 'NoDefaultType', 'EnumRegistry']. Creating it and re-basing EnumRegistry changes the MRO of all 124 registry enums, so it should land alone, with nothing else in the change.

Two measured traps for whoever writes the "immutable" test: CommandType._value2member_map_ is not fixed — it holds 6: P|S at import and gains 5: A|S on first CommandType(5) while __members__ stays at 4 — and the same applies to the 6 httpv2 Flags, for Flag reasons that are not minting. Also test_const_enum_no_mint.py:1974 asserts assertFalse(issubclass(CommandType, EnumRegistry)), which needs re-reading against any new base.

Not verified: rows 2-3 (PacketDirection, PacketReception) rest on bit-widths derived from their member values, not on the pcapng draft's text — no network was used, so someone should check §4.3 epb_flags before acting on those two. No test file was run, so every "tests would break" claim is reference-mapping, not an observed failure.

[JarryShaw]

For ProcedureCode and ProtocolIE, since their source is from pycreate, we can directly 1) read from the Python object at runtime; or 2) read from its GitHub or PyPI distributed sources. We had Vendor.DATA for that.

For TLSKeyLabel, what's your recommendation? We should work aiming for future. So I guess it should stay open?

For TransportProtocol and alike, reparenting anyways - we're enforcing and establishing a convention here.

Any additional items I should make a ruling?

[JarryShaw]

Recorded, verbatim:

For ProcedureCode and ProtocolIE, since their source is from pycreate, we can directly 1) read from the Python object at runtime; or 2) read from its GitHub or PyPI distributed sources. We had Vendor.DATA for that.

For TLSKeyLabel, what's your recommendation? We should work aiming for future. So I guess it should stay open?

For TransportProtocol and alike, reparenting anyways - we're enforcing and establishing a convention here.

Any additional items I should make a ruling?

On Vendor.DATA — one correction, because it changes which of your two options is the real win. There is no DATA attribute on Vendor; grep -n DATA pcapkit/vendor/default.py returns nothing. It is a per-crawler convention: a module-level DATA dict plus a request() override returning it, used by at least 8 crawlers (pcapng/secrets_type.py, pcapng/verdict_type.py, ipx/socket.py, tcp/flags.py, …). So option (2) — vendoring pycrate's table as a DATA literal — would move the hand-maintenance from the const file into the vendor file without removing it. Option (1) is the one that actually automates: a request() that imports pycrate at generation time and reads NGAP_Constants live. Cost to weigh: it makes pycrate a generation-time dependency, and the emitted table then varies with whichever pycrate the generator ran against — which wants recording in the generated header.

TLSKeyLabel: yes, open — my recommendation, and it costs nothing structurally. The NSS SSLKEYLOGFILE label set has already grown once within this very class (RSA/CLIENT_RANDOM are the SSL-era pair; the other seven are TLS 1.3 secrets), and pcapng's secrets block merely carries the label text, so an unrecognised label is a decode-time fact rather than an error. And it needs no new machinery: 5 registries already mix EnumRegistry with StrEnum (measured — 110 IntEnum, 5 StrEnum, 5 IntFlag), so this is a precedented shape, not a first.

TransportProtocol and alike: understood, reparenting all of them. That overrides the audit's "leave alone" recommendation for TransportProtocol and CommandType — convention beats blast radius, and the radius is test-file churn rather than behaviour.

Five things I would still want a ruling on, in the order they block work:

1. The four mh.py enums — the decision that unblocks fix: six .get implementations mint unknown keys at a shared -1, so the second aliases to the first #880. They cite "IANA keeps no registry of them" yet each reads an 8-bit wire field, so an unnamed value is something a real capture can carry. Should _missing_ raise, or return a non-registering _unregistered_member (closed registry, tolerant parser)? I lean tolerant, since a parser that raises on a byte it merely does not recognise is worse than one that labels it.
2. Does get case-fold? The base is case-sensitive (LinkType.get('ethernet') → KeyError) while TransportProtocol.get('TCP') succeeds by lowercasing internally. Reparenting forces a choice, and either direction breaks a caller.
3. get's exception types — KeyError for a name miss, ValueError for a value miss, and neither from pcapkit.utilities.exceptions as the house rule requires. Worth fixing while the base is being written.
4. The six nested httpv2 Flags — same convention presumably, but note their _value2member_map_ grows by Flag composite caching without minting, as CommandType's does. Any "immutable means the map never grows" test will fail on all seven for reasons that are not minting.
5. PacketDirection / PacketReception — I marked them closed from their member values, not from the pcapng draft's text (no network was used). Someone should read §4.3 epb_flags before they are declared closed, since "reserved" and "undefined" point different ways.