feat(reg): expose AppType aliases in repr/str and allow registering them per transport (#807) - #837
Conversation
|
NEEDS CHANGES — one finding. The three requirements are met and the per-transport semantics are exactly as ruled, but a registered alias cannot be looked up by the name it was registered under. Verified on The finding. An alias whose name cannot resolve is most of the point of an alias. And it departs from this file's own convention — The deliberate divergence from my brief is correct, and I checked it rather than taking it on trust. I had pointed at Also confirmed: the four per-transport const files need no changes; |
fd2b9b9 to
976c949
Compare
|
Finding fixed in The normalisation matches One trap it caught and fixed on its own, worth recording because #775 will hit it across 113 registries. Its first draft put Generator/const agreement re-verified by rendering Head is new so no verdict carries: |
|
NEEDS CHANGES — cross-review (opus, a different model from the author) on F1 — the inline F2 — Confirmed, and it settles the sequencing question: The 52.65% → 53.59% coverage figures are UNVERIFIED (budget spent on F1); direction is not in doubt. |
fe543c3 to
7898476
Compare
|
Both findings fixed in
Verified on the pushed tree (scratch worktree at the head,
Head is new, so the previous verdict does not carry: back to |
…hem per transport (#807) - Add a public `.aliases` property on every `AppType` member. No new storage: derived as `__registry__.getlist(self.port)` minus `self`, which is exactly the bucket `get_all` already walks -- an empty tuple when nothing collides, never `None`. - Grow `__repr__`/`__str__` an `(aliases: ...)` suffix when `.aliases` is non-empty; unaliased members (the vast majority) render byte-identical to before. `_value_` deliberately stays untouched, since it is the live key in `_value2member_map_` and aliases are registerable at runtime. - Add `register_alias(port, name)`, a classmethod scoped to one per-transport registry at a time -- `TCP.register_alias(...)` never touches `UDP`'s or `SCTP`'s members, matching the maintainer's ruling that "alias should ... be per-transport". Requires a canonical member already on `port`; mints via the same `extend_enum` call `get`'s own mint-on-miss already uses. - The Python attribute name is sanitised from `name` the same way `Vendor.safe_name` (pcapkit/vendor/default.py) sanitises every statically declared member's -- `www-http` becomes `www_http` -- so `TCP.register_alias(80, 'http-alt-test')` lands as `TCP['http_alt_test']` with `svc` still reading the unsanitised string. Refuses explicitly, never with a synthetic fallback, when the name has no valid identifier left after sanitising (empty, or leading with a digit) or when the identifier collides with an unrelated existing member. - Applied to the vendor generator template identically, verified by rendering `BASE(...)` with the real TABLE/MISS blobs extracted from the committed const file and diffing -- the only differences are the intended new code. The four per-transport const files need no changes: none of them override `__repr__`/`__str__`, and `.aliases`/ `register_alias` are inherited unchanged from the base. - Updated the two existing sweeps over all 12,391 members (`test_the_new_dunders_are_byte_identical_to_the_percent_form`, `test_every_member_renders_its_own_registrys_transport_protocol`) to account for the new suffix, and added tests for `.aliases`, the repr/str suffix, and every `register_alias` success/error path, including the sanitised-lookup, collision, and invalid-identifier cases. Build/test: `tests/const/`, `tests/foundation/registry/` and `tests/vendor/` pass (187 tests, 39,468+ subtests). Coverage of the touched const file rose from 52.65% to 53.59% (855/1624 -> 888/1657 lines), with 0 of the new statements missed.
7898476 to
c98ac3c
Compare
|
GOOD TO GO — delta cross-review (fable, a model distinct from both the sonnet author and the earlier opus reviewer) on What it derived independently, and I re-derived myself before posting:
The nit, which was correct and is now fixed. The body Head moved, so the verdict does not carry yet: the same reviewer is re-checking the one-word delta on |
|
GOOD TO GO on
Setting |
|
Ready to merge. CI on No merge-into- Gates, all closed: single commit authored One thing still open and deliberately not claimed: the PR body's coverage figures 52.65% → 53.59% were never reproduced by either reviewer. The direction is not in doubt — 156 lines added to the const file with 10 new tests exercising them — but the two exact percentages are unverified, so treat them as approximate. Unmerged and awaiting you. #657 remains last by design. |
|
Coverage claim now verified, and it exposed a stale figure in the PR body, which I have corrected. Measured on
Tests 25 → 35 in The PR body was wrong and is now fixed. It read Two methodology notes, since a coverage number in this repo is easy to get wrong:
|
Please follow the guide below
make mypy,make isort; ran both directly against the changed files)make testpasses, and a test case covers the change (scoped totests/const/,tests/foundation/registry/,tests/vendor/per the affected trees;tests/dumpkit/also run and shows six pre-existing, unrelated failures from missing generated sample captures)docs/source/changelog/— N/A, changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657What is the purpose of your pull request?
feat— adds a featureDescription of your pull request and other information
Implements #807, step 2 of #801's three: a public
.aliasesproperty,repr/strexposing aliases when there are any, and a way to register an alias on a per-transport registry..aliases. No new storage:AppType.__registry__.getlist(self.port)minusselfis exactly the bucketget_allalready walks to answer with the aliases after the canonical member, so.aliasesis that same bucket. Returns an emptytuple— neverNone— when nothing else collides.TCP.http.aliasesgives(TCP.www, TCP.www_http);SCTP.get(80).aliasesis(), because IANA never registeredwwwon SCTP.repr/str. Grow an(aliases: ...)suffix, computed fresh on every call, only when.aliasesis non-empty:_value_is deliberately untouched — it is the live key in_value2member_map_, built once at class creation, and aliases are registerable at runtime, so baking the list into_value_would go stale the moment one is registered.str(member) == member.valueholds when a member is minted and stops holding the moment a sibling is later registered as its alias.Registering an alias:
register_alias(port, name), a classmethod scoped to one per-transport registry. The maintainer's own signature sketches were explicitly "just POC not how it should actually look like," and his final ruling was the operative one: "alias should thus be per-transport, and we should probably allow registering aliases for a given enum in the transport apptype enum." So the mechanism lives directly onTCP/UDP/SCTP/DCCPrather than on a shared entry point that takes a transport argument —TCP.register_alias(...)cannot touchUDP's orSCTP's members by construction, which is a stronger guarantee than aproto=keyword would give and matches how.aliasesitself is already per-registry. It requiresportto already carry a member of that registry (aliasing names a second service on a port that already has one, rather than declaring a fresh one out of nothing — that stays the generator's job, orget's own mint-on-miss), and it refuses anamealready registered on that port. It mints through the sameextend_enumcallget's mint-on-miss fallback already uses, with the Python attribute name derived from the port and a counter rather than fromname— sanitising arbitrary caller text into an identifier would duplicate the generator's own curated dedup logic for a case nothing in the library needs, since no lookup path reaches an alias by its attribute name.pcapkit/foundation/registry/protocols.pyis deliberately left untouched. Itsregister_apptypeonly ever writes to{tcp, udp}protocol-class registries (pcapkit.protocols.transport.{tcp,udp}, a different pair of registries entirely frompcapkit.const.reg.apptype), so analiases=keyword there would be strictly weaker than the general, all-four-transportregister_aliasabove, and would conflate registering a protocol handler with registering an enum member.Generator agreement, verified rather than asserted. Applied identically to the vendor template's
BASElambda. Verified by renderingBASE(NAME, DOCS, FLAG, TABLE, MISS, MODL)with the realTABLE/MISSblobs extracted from the previously-committed const file (by template-matching the oldBASEagainst it) and diffing the result against the committed file end to end — the only differences are the newaliasesproperty, the newregister_aliasclassmethod, and the__repr__/__str__bodies, with the 59,366-character docstring table and the 134,846-character_missing_branch list byte-identical either side. The four per-transport const files (tcp.py,udp.py,sctp.py,dccp.py) need no changes: none of them override__repr__,__str__, or declare their ownaliases/register_alias, so all three are inherited unchanged from the base.Two existing sweeps over all 12,391 members (
test_the_new_dunders_are_byte_identical_to_the_percent_form,test_every_member_renders_its_own_registrys_transport_protocol) pinned the pre-#807repr/strtext for every member, aliased or not, and would otherwise fail for the ~94 members on the 44 colliding ports. Both are updated to append the identical suffix throughmember.aliasesbefore comparing, while keeping the_value_/lookup round-trip checks on the un-suffixed form.Six new tests cover
.aliases, the repr/str suffix (with and without aliases), and everyregister_aliaspath — success, called on the base registry, called on a port with no canonical member yet, and a duplicate name. Each was confirmed to fail against the pre-fix code with the expectedAttributeError/AssertionErrorbefore the fix landed, and to pass after.Coverage of
pcapkit/const/reg/apptype/apptype.py, measured with a standalonecoveragercpointingsourceat this worktree's absolutepcapkitpath and a distinctCOVERAGE_FILEper run (coverage runundertests/const/test_const_apptype_split_unit.py, filtered atcoverage report --include=...time): 52.65% (855/1624 statements) before this change's tests, 53.59% (888/1657) after — all 33 new statements covered, the pre-existing 769 misses (unrelated dunder comparisons and_missing_branches never exercised) unchanged in count.tests/const/,tests/foundation/registry/andtests/vendor/pass in full: 187 tests, 39,468 subtests.tests/dumpkit/was also run per the affected-trees list; six failures there are pre-existing and unrelated —FileNotFoundErrorfor generated sample captures (test.pcapngetc.) that this checkout never generated, in a test file with no reference toAppTypeorTransportProtocol.