Skip to content

feat(registry): foundation.registry wrappers for IPX and SCTP sub-maps; record the convention (#2511) - #2521

Open
JarryShaw wants to merge 3 commits into
mainfrom
feat/2511-registry-wrappers
Open

JarryShaw wants to merge 3 commits into
mainfrom
feat/2511-registry-wrappers

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

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
  • release — bumps the version or rolls up a distribution
  • chore — anything else
  • const — regenerates constant or registry tables, members keeping their numeric values

Description of your pull request and other information

Part of #2511. The LinuxSLL, FRAD and NETLINK maps are covered by #2510.

  • register_ipx, register_sctp_chunk, register_sctp_parameter and register_sctp_cause are added to foundation.registry.protocols. Each follows the existing wrappers: a # NOTE: <map> tag, _check_code and a target check, the classmethod call, and _register_name for protocol maps. They are exported and documented alongside the others.
  • registry-protocol.rst has a new Registration Wrappers section. It states the two-part shape and the exception for inherited classmethods (owner's option 2 on refactor(registry): every class-level registry map gets its foundation.registry wrapper #2511).
  • ext.rst: the SCTP and ICMP extension-point rows now name the wrappers.
  • A new test pins every class-level map to its wrapper and fails without this change. Its excused table lists the maps other streams own.

Loopback's wrapper is deferred to the link-layer section, which #2510 is editing. Its int keys raise a question that is posted here once the review lands.

…convention (#2511)

- add register_ipx for IPX.__proto__, and register_sctp_chunk,
  register_sctp_parameter and register_sctp_cause for SCTP's chunk,
  parameter and cause maps, each shaped like its siblings: _check_code,
  a parser-name or protocol check, the owning classmethod, a debug log,
  _register_name for the __proto__ map, schema= for the parser maps
- export them from pcapkit.foundation.registry, pcapkit.foundation and
  pcapkit.all; document them on the registry page and in ext.rst, whose
  ICMP and SCTP extension-point rows now name the wrappers
- record the two-part shape as "Registration Wrappers" on the
  registry-protocol conventions page
- add test_registry_wrapper_coverage_2511_unit, which fails on a protocol
  class map no wrapper's "# NOTE:" tag names unless excused, and case the
  new wrappers in the code-check, symmetry and wrapper-route suites
@JarryShaw JarryShaw added feat Pull requests that add a new capability (feat: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Nov 1, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Nov 1, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review (Sonnet) at d5d81d620: NEEDS CHANGES. What is here is correct: the inventory is complete, the four wrappers match their siblings in shape, exports and docs, and the tests fail without the change. But the owner's option 2 on #2511 assigns more to this PR than it delivers:

  • IPX needs its own register, because the inherited Internet.register is TransType-typed. Loopback needs both a register classmethod and a register_loopback wrapper.
  • The Registration Wrappers section has to state the inherited-classmethod exception. As written, it contradicts IPX as shipped.
  • EXCUSED attributes Loopback to refactor(link): dispatch LinuxSLL's non-EtherType ARPHRD values through its own registry map #2510, which does not own it.
  • The coverage test pins only the wrapper half, so it cannot catch a missing classmethod.

Fix round dispatched. It is pushed once queued runs drop below 20. Loopback's int keys raise the open question posted on #2510.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Nov 1, 2026
…r_loopback (#2511)

- IPX and Loopback define their own register classmethod, since the inherited
  Internet.register and Link.register are typed for TransType and EtherType.
- IPX declares __proto_key__ = (Packet, True), so a code= key is checked as
  register_ipx checks its own; register_ipx also takes a bare int now.
- register_loopback joins the link-layer wrappers, exported and documented
  beside its siblings; _check_code takes kind=None for an int-only check.
- The coverage test drops Loopback from EXCUSED and pins the classmethod
  half, listing TCP's and UDP's inherited Transport.register in INHERITED.
- registry-protocol.rst states the inherited-classmethod exception and the
  __proto__ wrapper naming rule.

Registry and protocol-code test modules pass; the new pins fail without the fix.
@JarryShaw

Copy link
Copy Markdown
Owner Author

The fix round is done locally (e451c70cb, not pushed yet). It adds IPX.register, Loopback.register, register_loopback, _check_code(kind=None) and a classmethod-half coverage pin, plus the option-2 and naming text. Its review asks for one more small round, now running: the naming rule as written is false for register_apptype, and IPX lost a junk-code pin. Decisions:

  1. IPX's __proto_key__.
    • (a) No pairing, as on main. Any key is accepted through code=, including TransType.UDP, which then keys Packet.NCP.
    • (b) (Packet, False), like Internet and LinuxSLL. That breaks 1.5.0's accepted {IPX: 4}.
    • (c) (Packet, True), with register_ipx taking ints. It keeps 1.5.0's bare-int keys, rejects other enums and bools, and mirrors SCTP.
      Recommendation: (c), which is what the branch does.
  2. The naming rule's exact wording (your refactor(link): dispatch LinuxSLL's non-EtherType ARPHRD values through its own registry (#2510) #2527 Q4 ruling). Read literally, it fails for register_pcap/register_pcapng, and for register_apptype, which takes bare ints but writes both TCP's and UDP's maps. Options: (a) state the rule with those named exceptions; (b) rename them, which is breaking. Recommendation: (a).

@JarryShaw JarryShaw added the needs: decision Waiting on the maintainer to decide — not blocked by other work label Nov 1, 2026
…PX's junk-code pin (#2511)

- registry-protocol.rst names every __proto__ wrapper departing from the
  naming rule (register_pcap, register_pcapng, register_apptype) with its
  reason, and says Internet.register and Link.register are the parents' types.
- The code-check suite again rejects None and a str code for every number
  registrar, register_ipx included; it fails if _check_code lets them through.
- IPX's __proto_key__ doc-comment states the bare-int key without history.
@JarryShaw

Copy link
Copy Markdown
Owner Author
  1. C
  2. A

@JarryShaw JarryShaw moved this from In review to Needs decision in PyPCAPKit Nov 1, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Owner chose 1(c) and 2(a). IPX declares __proto_key__ = (Packet, True) and register_ipx takes ints, which keeps 1.5.0's bare-int keys and rejects other enums and bools, as the branch already does. The naming rule is stated with its exceptions named (register_pcap, register_pcapng, register_apptype, and any other the check finds). The second fix round, now running, writes exactly that.

@JarryShaw JarryShaw removed the needs: decision Waiting on the maintainer to decide — not blocked by other work label Nov 1, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Fix rounds at 5b2833f59, cross-reviewed (Sonnet, round 2): GOOD TO GO.

  • IPX.register and Loopback.register mirror Link/Internet.register with per-key warnings, and register_loopback is added. Removing either classmethod fails the tests.
  • The naming rule's exceptions (register_pcap, register_pcapng, register_apptype) and the protocol-wide registrars are stated on registry-protocol.rst. An AST pass over every wrapper matches them.
  • A None/str mutation fails the new subtests, and every added line in protocols.py, ipx.py and loopback.py is covered.

Non-blocking: the "tagged # NOTE" wording does not fit three wrappers, the two new classmethods have no versionadded, and one import is out of isort order.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat Pull requests that add a new capability (feat: subject prefix) review: good-to-go Cross-review at the current head says ready; CI state is separate

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

1 participant