Skip to content

refactor(corekit)!: move the schema helpers protocols call into corekit.schema (#2674) - #2704

Merged
JarryShaw merged 1 commit into
mainfrom
refactor/2674-helper-move
Nov 3, 2026
Merged

refactor(corekit)!: move the schema helpers protocols call into corekit.schema (#2674)#2704
JarryShaw merged 1 commit into
mainfrom
refactor/2674-helper-move

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Nov 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • refactor — changes neither behaviour nor performance

Description of your pull request and other information

The owner ruled on #2674, refining it in session: helpers shared beyond the schema layer live in one package, pcapkit.corekit.schema, with a submodule per group. A protocol-specific helper moves only if code outside its own protocol uses it and it is generic enough to reuse. No old path keeps a re-export.

Submodule Helpers Moved from
padding pad_length corekit.fields.strings
ipaddress parse_ip_address and its guards corekit.fields.ipaddress
byteorder packet_byteorder, byteorder_callback (the whole module) corekit.fields.byteorder
jumbogram jumbo_payload_length the IPv6 schema
shortread keep_short_read, replay_short_read protocols.schema.schema

The package holds shared helpers. It is not the Schema framework in pcapkit.protocols.schema, and each module's docstring says so. Shipped names get versionremoved at their old paths. The ruling is recorded on utilities-or-corekit.rst under "Shared Schema Helpers". A boundary test fails when code outside the schema layer, or outside a protocol, calls a helper from a field module, schema.schema or another protocol's schema module. It also fails on a helper defined twice, and on an import cycle into the package.

Breaking: names shipped since 1.5.0 move with no re-export.

All 13 legs pass, every module imports, and the docs build gives the same warnings as main.

Closes #2674

@JarryShaw JarryShaw added this to the PyPCAPKit v1.6 milestone Nov 2, 2026
@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Nov 2, 2026
@JarryShaw JarryShaw mentioned this pull request Nov 2, 2026
Closed
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Nov 2, 2026
@JarryShaw
JarryShaw force-pushed the refactor/2674-helper-move branch from 096f76f to ffbd611 Compare November 2, 2026 23:28
@JarryShaw

JarryShaw commented Nov 2, 2026

Copy link
Copy Markdown
Owner Author

Rebased 096f76fab→ffbd611b8 onto 92ecf2ffa.

  • One conflict, resolved keeping both sides. It was in the PCAP-NG schema's import block: main's hold_warnings (fix(misc): let the built-in record a held ZigBee key's short read (#2688) #2697) plus this PR's pad_length from pcapkit.utilities.padding.
  • Nothing to update from the merges. No merged PR added an old-path use of a moved name; the boundary test and an AST scan both confirm it.
  • Checks: every module imports, and the corekit, internet, misc and link legs pass.

The earlier review was cut off by a credential failure, so a Sonnet re-review is running on this head.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Nov 3, 2026
@JarryShaw

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

GOOD TO GO at ffbd611b8, from a Sonnet cross-review.

  • The move is clean: all eight moved functions are AST-identical to their old definitions, each placement passes utilities-or-corekit.rst's test, and nothing is re-exported.
  • Version notes: versionremoved appears exactly where a name shipped in v1.5.1.
  • Nothing else qualifies to move.
  • No regressions: all 686 modules import, and the corekit, utilities, link, misc, transport and internet modules pass.

One small test-only follow-up goes into this PR, and I'll check the amend: from pcapkit.corekit.fields import ipaddress followed by ipaddress.parse_ip_address(...) slips past the boundary test, because the package itself is a home.

@JarryShaw JarryShaw mentioned this pull request Nov 3, 2026
Closed
@JarryShaw
JarryShaw force-pushed the refactor/2674-helper-move branch from ffbd611 to dd6072a Compare November 3, 2026 00:15
@JarryShaw

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

Pushed ffbd611b8→dd6072acc, rebased onto 57a6c366d with no conflicts. I checked the delta myself; it is test-only:

  • The boundary scanner records a function only when the importing package itself defines it. A submodule or re-export reached through a package that is a home is now caught.
  • A new test covers a plain submodule import, an aliased one and a re-export, and it fails without the fix.

The schema, foundation, application, corekit and misc legs pass, and every module imports. This stays GOOD TO GO.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment needs: decision Waiting on the maintainer to decide — not blocked by other work and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Nov 3, 2026
@JarryShaw

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

The owner asked in session, paraphrased, to group the moved helpers in one subpackage instead of splitting them across pcapkit.corekit and pcapkit.utilities. That subpackage would sit under the fields package: corekit.jumbogram and corekit.shortread, plus utilities.byteorder, utilities.ipaddress and utilities.padding, would all move there. This replaces the current layout, so the verdict moves to review: needs-changes.

Needs deciding: the subpackage's name.

  1. pcapkit.corekit.fields.utilities, as suggested.
  2. pcapkit.corekit.fields.helpers.
  3. pcapkit.corekit.helpers, a sibling of fields. Two of the five (shortread acting on protocols, jumbogram on IPv6) are not field helpers.

Recommendation: 2. It keeps the location you asked for, but fields.utilities would read as a second pcapkit.utilities, whose import rule (it imports nothing from pcapkit) these modules don't follow. I'm going ahead with 2, rewriting utilities-or-corekit.rst and the boundary test to match. A different name is a mechanical rename.

@JarryShaw

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

The owner then proposed pcapkit.corekit.schema, with one submodule per helper group. It fits better than the three options above, since the five are schema-layer helpers, not field types: byteorder, ipaddress, padding, jumbogram and shortread. Going with it.

One naming overlap, for the record: pcapkit.protocols.schema remains the Schema framework, and after #2534 it is the module protocols/schema.py. Each module docstring and docs page says which is which. needs: decision comes off; it stays review: needs-changes until the restructure lands.

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

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

The owner adds, paraphrased: the whole of pcapkit.corekit.fields.byteorder moves to pcapkit.corekit.schema.byteorder. That takes byteorder_callback as well as packet_byteorder, and the old module is removed with no re-export. Both names shipped, so versionremoved goes at the old path. The field classes then import byteorder_callback from its new home. This is folded into the same restructure.

@JarryShaw
JarryShaw force-pushed the refactor/2674-helper-move branch from dd6072a to a6efa73 Compare November 3, 2026 01:01
@JarryShaw JarryShaw added the review: running A cross-review is in flight against the current head - no verdict yet label Nov 3, 2026
@JarryShaw JarryShaw changed the title refactor(corekit)!: move the schema helpers protocols call into corekit and utilities (#2674) refactor(corekit)!: move the schema helpers protocols call into corekit.schema (#2674) Nov 3, 2026
@JarryShaw JarryShaw removed the review: needs-changes Cross-review at the current head says changes are required; see the verdict comment label Nov 3, 2026
@JarryShaw

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

Pushed the restructure, dd6072acc→a6efa7351. Every moved helper now lives in a pcapkit.corekit.schema submodule (padding, ipaddress, byteorder, jumbogram, shortread), and all of corekit.fields.byteorder moved there. The interim utilities.* and corekit.* modules are gone; they never shipped. A Sonnet re-review is running.

@JarryShaw

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

GOOD TO GO at a6efa73 (Sonnet re-review).

  • Every moved helper is AST-identical to main with docstrings stripped. No old path is still referenced in pcapkit/, docs/, tests/, examples/ or util/.
  • Every module imports cleanly in a fresh process. The corekit.schema modules only reach protocols and fields under TYPE_CHECKING.
  • The version notes check out against v1.5.0 and v1.5.1, and the boundary test catches the duplicate-definition and import-cycle mutants.
  • Follow-up, folding in now as a test-only change: imported_names misses from pcapkit.corekit import fields inside a corekit.schema module.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Nov 3, 2026
@JarryShaw
refactor(corekit)!: move the schema helpers protocols call into corek…
a2a3f6f 
…it.schema (#2674)

- The new pcapkit.corekit.schema package holds the helpers shared across
  the schema layer, one module per group; its __init__ imports none of
  them, and none imports a field module or a protocol at run time.
- pad_length moves from pcapkit.corekit.fields.strings to .padding.
- parse_ip_address, with the _reject_bool and _reraise_as_field_value_error
  guards the IP address fields share, moves from
  pcapkit.corekit.fields.ipaddress to .ipaddress.
- pcapkit.corekit.fields.byteorder moves whole, with byteorder_callback and
  packet_byteorder, to .byteorder.
- keep_short_read and replay_short_read move from
  pcapkit.protocols.schema.schema to .shortread.
- jumbo_payload_length, which the IPv6 pseudo-header and the Scapy toolkit
  call as well as IPv6, moves from pcapkit.protocols.schema.internet.ipv6
  to .jumbogram.
- Every caller, radiotap's included, imports from the new home, with no
  re-export at the old path; the shipped names get versionremoved there.
- utilities-or-corekit.rst records the rulings, and
  test_schema_helpers_boundary_2674_unit fails on a schema-layer helper
  called from outside the layer, or from outside its own protocol.

Every tests/ leg passes under util/run_unittest_leg.py, and the Sphinx
build warns exactly as the base does.
@JarryShaw
JarryShaw force-pushed the refactor/2674-helper-move branch from a6efa73 to a2a3f6f Compare November 3, 2026 01:47
@JarryShaw

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

Fold pushed: a6efa7351→a2a3f6fdb, still one commit on 57a6c366d. The change is test-only, with no diff under pcapkit/ or docs/.

  • imported_names now also yields <target>.<alias>, so from pcapkit.corekit import fields resolves to pcapkit.corekit.fields. New cases pin both the walker and the corekit.schema case. Run with the old walker, the helper case fails.
  • The failure message now names pcapkit.corekit.schema.
  • The boundary, conventions and import tests pass locally. The verdict stays GOOD TO GO, since only tests changed after the reviewed head.

@JarryShaw
JarryShaw merged commit bfa3423 into main Nov 3, 2026
59 checks passed
@JarryShaw
JarryShaw deleted the refactor/2674-helper-move branch November 3, 2026 02:11
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Nov 3, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Nov 3, 2026
JarryShaw added a commit that referenced this pull request Nov 3, 2026
@JarryShaw
docs(changelog): fold make()'s enumeration fields, the corekit.schema…
d2aee5b 
… helpers, MH options 6 and 27, the cut SCTP and option-area schemas and dict-built secrets into 1.6.0 (#2449)

- Breaking: a make() enumeration field takes a member or an int, its
  X_default/X_namespace/X_reversed keywords removed with no alias, and names,
  wide codes and wide option codes are refused (#2692); the schema helpers
  move to pcapkit.corekit.schema and pcapkit.corekit.fields.byteorder is
  removed (#2704); MobileNetworkPrefixOption gains prefix_length and keeps an
  IPv6Address prefix, and TimestampOption's timestamp is None past 9999
  (#2709). Folded into the MH data-model entry; old values measured against
  v1.5.1.
- Fixed: the SCTP schema cut inside its padding and a header schema cut
  inside an option area pack back as captured (#2682), into the SCTP entry;
  a Decryption Secrets Block rebuilds from its dict as captured (#2707), into
  the DSB entry.
- Folded into unreleased entries: FRAD's Q.922 address refusal and quiet cut
  rebuild (#2719); a from_data rebuild's checksum status (#2712); a failed
  ListField fill's ledger (#2706); pad_length's path (#2704); TLS's enumeration
  fields (#2692); the make() and schema-helper rulings, into the conventions
  entry (#2692, #2704); the GraalPy shards, into the CI-legs entry (#2720).
- 1.5.0's corekit.fields.byteorder role points at corekit.schema.byteorder,
  where #2704 moved it, so the nitpicky build does not warn on it; 1.5.0's
  text unchanged.
- No entry for the test-only 72b2bd8 (#2715), or the docs-and-test-only
  0c07940 (#2718).
- CHANGELOG.md regenerated; util/changelog_md.py --check passes.
This was referenced Nov 3, 2026
Merged
Merged
JarryShaw added a commit that referenced this pull request Nov 3, 2026
@JarryShaw
docs: resolve the pad_length, _make_uint and copyreg references left by
6b63ee9 
#2704 and #2722

- protocol.rst linked pad_length at pcapkit.corekit.fields.strings, its home
  before #2704; it links pcapkit.corekit.schema.padding now.
- protocol.rst documented Protocol._make_uint twice, beside _make_index and
  among the value helpers; the value-helper entry stays, in the Protocol
  autoclass, the ProtocolBase method group and its link list alike.
- ReduceAsProtocol2 names copyreg._reduce_ex and copyreg.__newobj__ as
  literals, since intersphinx holds neither private name.
- HIP and IPv6_Route filed their #2676 note under versionremoved 1.6.0; it
  moves, unreworded, into each class's versionchanged 1.6.0 list.

Docstrings and .rst only; the AST is equal once docstrings are stripped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix)

Projects

PyPCAPKit
Status: Done

Milestone

PyPCAPKit v1.6

Development

Successfully merging this pull request may close these issues.

refactor: move helpers shared outside the schema layer into corekit or utilities

1 participant

@JarryShaw