Skip to content

refactor(corekit): give EnumRegistry a bare EnumLookup parent carrying get/get_all (#877) - #906

Merged
JarryShaw merged 1 commit into
mainfrom
refactor/877-enum-lookup-base
Sep 29, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
refactor/877-enum-lookup-base

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • chore — anything else

Description of your pull request and other information

Phase 1 of #877, and only phase 1 — re-parenting the helper enumerations onto the new base is phase 2 and is deliberately not here, because it touches files three other changes are currently in.

EnumLookup now carries get, get_all and a new _validate_value; EnumRegistry(EnumLookup) keeps register, register_alias, register_aliases, _extend and _unregistered_member. That split, and the choice not to put register on the base, are both the owner's rulings on #877 — the second following his own "if it carries register, then why not register_alias".

_validate_value is the range-validation hook asked for there. The base accepts everything, so nothing changes for a class that does not override it; get calls it just before cls(key) and register after its duplicate check.

Behaviour-preserving, measured rather than asserted. A fingerprint of all 125 registries — members, lookup tables, get/get_all/default over every member, miss paths, and pickle on all six protocols — is byte-identical before and after, apart from the MRO and method-resolution strings. pcapkit/corekit/enum.py stays at 100% coverage with covered statements rising 62 → 67; pylint, mypy and vermin are at exact parity.

Also corrected this module's own census — 124 → 125 registries, with TLSKeyLabel added to its str-valued list — both stale since #890. Per-registry case-sensitivity auditing is #903.

…g get/get_all (#877)

`pcapkit/corekit/enum.py` held a single base, so an enumeration wanting only
`get`/`get_all` had to inherit `register`, `register_alias`, `register_aliases`,
`_extend` and `_unregistered_member` with it -- a mutating contract the owner
ruled the closed helper sets must not be handed: "they may subclass a bare base
enum from pcapkit.corekit.enum - where EnumRegistry subclasses it for using in
the other mutable ones."

- `EnumLookup` now carries `get`, `get_all` and a new `_validate_value`, and
  `EnumRegistry(EnumLookup)` keeps the mutating five. Which tier each method
  lands on follows the owner's own second thought -- "if it carries `register`,
  then why not `register_alias`" -- because a base holding both leaves
  `EnumRegistry` too thin to justify being a second class at all.
- `_validate_value` is the range-validation hook the owner asked for. The base
  accepts everything, so nothing changes for a class that does not override it;
  `get` calls it immediately before `cls(key)`, and `register` after its
  duplicate check, so one override guards the lookup and the minting path both.
- Both tiers stay plain classes, so `_member_type_` still resolves past them to
  the enum base and `int`-, `str`- and flag-valued registries are untouched.
- Corrected this module's own census, 124 -> 125 registries, and added
  `TLSKeyLabel` to its str-valued list -- both stale since #890 moved that class
  from a hand-written helper to a generated registry.

Behaviour-preserving, measured rather than asserted: a fingerprint of all 125
registries -- members, lookup tables, `get`/`get_all`/`default` over every
member, miss paths, and pickle on all six protocols -- is byte-identical before
and after, apart from the MRO and method-resolution strings themselves.

Re-parenting the helper enumerations onto the new base is phase 2 of #877 and is
deliberately not in this change; auditing each registry's case sensitivity is
#903.
@JarryShaw JarryShaw added refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 29, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO on b686d7c23 — cross-review (sonnet; author opus). Clean on every claim, and the reviewer did something better than checking the tests exist: it mutation-tested them.

The hook is not decorative, proven by breaking it three ways — each reverted afterwards:

removed _validate_value() from register()      -> test_register_routes_through_the_hook FAILED
removed _validate_value() from get()'s value path -> test_the_str_key_path_does_not_call_the_hook  1 failure + 1 error
made get() case-fold                            -> 2 of 8 BareLookupTests FAILED

So the five new statements are genuinely pinned, not merely executed. It also built a real EnumRegistry+IntEnum subclass overriding _validate_value to reject out-of-range values and confirmed the override runs from both call sites, and that EnumValueError — being a ValueError subclass — is caught by get's own except ValueError and falls back to default correctly.

Behaviour preservation confirmed independently across all three member shapes, not taken from the author's fingerprint: _member_type_ resolves to str/int/int on a StrEnum, IntEnum and IntFlag registry, with EnumLookup as the only MRO insertion, diffed against origin/main. The string-key fall-through still resolves a value that is not a member name — measured on FEATCode, correctly noting the get override at pcapkit/const/ftp/command.py:407 belongs to Command, a different class in the same file, which is exactly the trap I fell into earlier with Method. The declared-but-unassigned asymmetry holds with the lookup tables unchanged at 15. All five mutating members keep byte-identical inspect.signature strings. Pickle round-trips with identity preserved across all six protocols on all three shapes.

The 125-registry census independently re-derived by walking pcapkit.const at runtime — 125, matching, with TLSKeyLabel correctly included at pcapkit/const/pcapng/tls_key_label.py:20. That is the number I had been quoting as 124 since #890 moved it.

#903's boundary respected: no per-registry audit in the diff. conventions.rst's new section defers to #903 and carries only the two already-settled rows rather than re-deriving them.

Not breaking, and the reviewer checked the thing that would make it so — a repo-wide sweep for __bases__/__mro__/isinstance(..., EnumRegistry) checks that an MRO insertion could disturb. The one hit, schema.py:1233's _EnumRegistry, is an unrelated locally-defined collections.defaultdict subclass — a name collision only. So no breaking label.

Merged-tree run (fast-forward, since the base is current main at 0981d771d), plain unittest so no subtest undercount: tests/corekit 271 tests OK with all 31 new ones verified by name rather than by total, tests/const 258 OK. Its first tests/const run showed 11 errors, all ModuleNotFoundError for requests/bs4 from a scratch venv missing the vendor extras — diagnosed as environmental, fixed, rerun clean. Worth noting because that is the failure mode that looks like a regression and is not.

Three things flagged from the author's own report, none blocking, all recorded rather than lost:

Flag registries do grow _value2member_map_ on a composite lookup — _Flag.get(99) caches first|second|96, while _member_names_ and __members__ stay untouched. That is your Flag carve-out rather than minting, but a blanket "closed sets never grow" assertion will be false for flags in phase 2. Recorded on #903.

pcapkit.corekit.enum has no docs page at all — absent from docs/source/pcapkit/corekit/index.rst's toctree, which I verified. So neither EnumRegistry nor the new EnumLookup is autodoc'd and every :class:~pcapkit.corekit.enum.*`` reference in conventions.rst is unresolved. Pre-existing and silent, because the build sets no `nitpicky` and uses no `-W`. Added to #902.

A merge-order hazard git will not flag. This PR renames EnumRegistry.get → EnumLookup.get in cross-references; #907 adds three new EnumRegistry.get references at tests/const/test_const_enum_no_mint.py:2267 and tests/const/test_const_method_case_sensitive_896_unit.py:25,29. The two PRs' conventions.rst hunks are ten lines apart and auto-merge cleanly, so nothing conflicts — whoever merges second just leaves three stale references. Mine to fix, in whichever order they land.

Ready for you to merge. I am not merging.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 29, 2026
@JarryShaw
JarryShaw merged commit c50f442 into main Sep 29, 2026
63 checks passed
@JarryShaw
JarryShaw deleted the refactor/877-enum-lookup-base branch September 29, 2026 04:54
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 29, 2026
JarryShaw added a commit that referenced this pull request Oct 3, 2026
…issue (#719)

- Nine citations in tests/ called a pull request "GitHub issue"
  (#921, #764, #906, #815, #936, #721, #501, #983) or lumped PR #428 in
  with issue #425; each now names the right kind.
- test_dispatch_default_resolution_unit: issue #425 reported the registry
  leak and PR #428 fixed it, so the sentence says "reported" and "fixed"
  instead of crediting the issue with the fix.
- Prose only: docstrings and comments, no assertion or logic touched.
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant