[JarryShaw]

• Searched for similar pull requests
• Followed the coding style (make pylint, make mypy, make isort)
• make test passes, and a test case covers the change
• Changelog entry: N/A — centralised in chore(docs): centralise changelog entries for unreleased versions #2483

What is the purpose of your pull request?

• refactor — changes neither behaviour nor performance

---

Description of your pull request and other information

Groups each protocol's protocol, schema and data modules into one package, for example pcapkit.protocols.internet.ipv4.{protocol,schema,data}. The codemod does it all: one command, no hand edits.

• The owner's rulings on refactor(protocols)!: group each protocol's protocol, schema and data into one package #2534 apply. The TLS data SSL reads moves to ssl/data.py, which removes the ssl.protocol → tls.data cycle. Each package __init__ exports its class only. Shipped names that move get versionremoved at their old paths, with no re-exports. The 1.5.0 changelog's references are rewritten.
• 156 files moved, 64 added and 20 removed, the removed ones being the old aggregator __init__s and two merged modules. Every other changed file is an import or reference rewrite.
• All 14 unittest legs and the project guards pass, including the docs: a doc comment whose first line has a colon renders its prose as a Type field #2745, docs: a module docstring's title underline runs six past the title #2718 and refactor: move helpers shared outside the schema layer into corekit or utilities #2674 guards, doc claims, member order, re-exports and both round-trip suites. 734 of 734 modules import. A clean-venv docs build has zero warnings. import pcapkit time is unchanged (0.637 → 0.640 s).
• The only conflict is with chore(docs): centralise changelog entries for unreleased versions #2483, in 1.5.0.rst. chore(docs): centralise changelog entries for unreleased versions #2483 merges last and takes the rewrite.

Closes #2534

[JarryShaw]

NEEDS CHANGES at e9de6cc5e, from a Sonnet review.

• Required: 15 names that shipped in v1.5.1 moved but have no versionremoved at their old documented path, unlike ESP. I confirmed MH: its page lists none of them.
  • internet.mh: FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LMAAddressCode, LocalizedRoutingStatus
  • misc.pcapng: PacketDirection, PacketReception, WireGuardKeyLabel
  • internet.ipv6: IPV6_HEADER_LEN, IPV6_NEXT_HEADER_OFFSET
  • ftp.Type, httpv1.Type, ngap.load_pycrate, esp.load_cryptography, sctp.CRC32C_TABLE, ftp.FTP_REQUEST/FTP_RESPONSE
• The fix goes in the codemod, with a re-run and no hand edits.
• Everything else holds: no import cycle, 889 records byte-identical, renames tracked, no coverage lost and the guards pass.
• A base pickle no longer loads (pcapkit.protocols.data.* is gone). That follows from the no-shim ruling and goes in chore(docs): centralise changelog entries for unreleased versions #2483's changelog.

[JarryShaw]

Round 2 pushed: e9de6cc5e → 92d3f631a, a fresh codemod run on main 05e113757 with no hand edits.

• The versionremoved notes are now derived from v1.5.1 itself by util/shipped_names.py, which writes the checked-in fixture tests/project/shipped_names_fixtures/v1.5.1.json. That covers the 16 names flagged above. It also covers hip.Locator, the private names v1.5.1 documented, and the aggregator re-exports. Re-exports now point at their defining module, e.g. ESP's Cipher/Integrity → pcapkit.const.esp.
• It also adds notes for 4 address patterns (PAT_MAC_ADDR, PAT_EUI_ADDR) that refactor(protocols): move shared value pack/unpack helpers into the Protocol utilities #2614 removed without one.
• New TestShippedNames in the refactor!: remove every surviving re-export of a moved name or module #1744 module: every shipped name resolves at its old path or a versionremoved names it. The guard reports 920 names on e9de6cc5e and passes here.
• _sibling_model is codemod output. All legs, guards and docs builds pass. A cross-review is running.

One question for you: see the next comment.

[JarryShaw]

Needs deciding: the #1744 test module says a removal is recorded twice, in the module docstring and on the docs page (#1103). The #2534 notes are on the docs pages only, and the guard accepts either.

1. Pages only. The old modules no longer exist, so their docstrings have nowhere to live, and the page is the only place that names the old path.
2. Pages plus a copy in each new module's docstring.

Recommendation: 1. The page is where readers look up an old path; option 2 doubles the text without a home for the old path. Going ahead with 1 unless you pick 2.

[JarryShaw]

take 1

[JarryShaw]

Ruling: option 1. The removal notes for #2534 live on the docs pages only, with no copies in module docstrings, and the guard accepts that as it stands. Nothing on the PR changes. The round-2 cross-review came back GOOD TO GO; the verdict follows once deploy-pages finishes, since Project tests has already passed.

[JarryShaw]

GOOD TO GO at 92d3f631a, from a Sonnet review of round 2.

• Coverage: an independent AST and docs scan of v1.5.1 found 3,218 public names. Every one resolves at its old path or is named by a versionremoved on the head, including all 16 from round 1.
• The fixture regenerates byte-identical and records its tag and commit. The guard fails on e9de6cc5e, and deleting one note line makes it fail naming that name.
• The notes name the old path with no alias and cite refactor(protocols)!: group each protocol's protocol, schema and data into one package #2534, refactor(protocols): move shared value pack/unpack helpers into the Protocol utilities #2614 or refactor: move helpers shared outside the schema layer into corekit or utilities #2674. Nothing under pcapkit/ changed since round 1. Project tests, Lint and deploy-pages pass.
• Non-blocking: hip.Locator exists only as a TYPE_CHECKING TypedDict, so its "in this module" note holds for stubs only.

[JarryShaw]

Needs deciding (raised in session): what to call the protocol module in each package, e.g. internet/ipv4/protocol.py or internet/ipv4/ipv4.py.

1. protocol.py, as the PR has it. Every package becomes the same trio: protocol.py, schema.py, data.py. _sibling_model and the codemod rely on that predictability. Callers import from the package (pcapkit.protocols.internet.ipv4.IPv4) and never see the module name.
2. <name>.py. Tracebacks and editor tabs are easier to tell apart, but you get stutter paths (ipv4.ipv4, tls.tls) and a trio with mixed naming. Switching means changing the codemod, re-running it (about an hour) and reviewing it again, since every versionremoved note and docs page names the .protocol paths.

Recommendation: 1. The paths users see are the same either way, and the trio stays regular. Tracebacks print the full path, so .../ipv4/protocol.py is still unambiguous. Going ahead with 1 unless you pick 2.

[github-actions]

Coverage: 94.45% (unit tier, Python 3.15, 9efd56dfc, Unit Tests run success)

Package	Statements	Missed	Branches	Partial	Cover
pcapkit (top level)	104	4	20	2	95.16%
pcapkit/const	20918	175	2908	156	98.57%
pcapkit/corekit	2889	76	928	37	96.83%
pcapkit/dumpkit	286	3	102	3	98.45%
pcapkit/foundation	3505	27	1216	32	98.75%
pcapkit/interface	138	4	52	3	96.32%
pcapkit/protocols	22785	155	5906	127	99.00%
pcapkit/toolkit	961	26	342	8	96.62%
pcapkit/utilities	559	7	150	5	98.31%
pcapkit/vendor	5590	2286	1280	214	55.75%

Per-file detail: the coverage-html artifact of this run.

[JarryShaw]

Ruling: keep protocol.py. Each package stays the protocol.py / schema.py / data.py trio, as the PR has it, so nothing changes for this question.

[JarryShaw]

Needs deciding (raised in session): should each package __init__ also import the schema and data classes, keeping __all__ to the protocol class only? The names collide: ipv4/protocol.py, schema.py and data.py each define IPv4, and the option classes repeat across schema and data. So an export needs aliases.

1. Headline classes, aliased: Schema_IPv4 and Data_IPv4, the alias names protocol.py already uses internally, outside __all__. In 1.5.1 these were reachable the same way, as module-level imports. This is a codemod change: re-run about an hour, then re-review.
2. No exports: pcapkit.protocols.internet.ipv4.schema.IPv4 and .data.IPv4 are one attribute away, so each class keeps one name, as the no-re-export rule (refactor!: remove every surviving re-export of a moved name or module #1744) prefers.

Recommendation: 2. It is the smaller surface and needs no re-run before the release. If you want 1, I'd do the headline classes only, with the guard taught about the aliases.

[JarryShaw]

GOOD TO GO at 9efd56dfc, from a Sonnet review of round 3.

• Every public class in each schema.py and data.py is bound in its package __init__ under the right prefix: 1115 aliases. Each alias is the module's own class, none is in __all__, and nothing else is bound. Classes defined only for Sphinx type-checking are correctly left out.
• Every package follows the protocol / # Header Schemas / # Data Models layout. from pcapkit.protocols.internet.ipv4 import IPv4, Data_IPv4, Schema_IPv4 works, and pickles keep the defining-module path.
• Swapping blocks, dropping an alias, or adding one to __all__ each fails a test that names the package. The only docs change is the conventions page.
• Project tests, Lint and deploy-pages pass.

The # isort: split question above is still open. Merging as is means keeping the marker.

[JarryShaw]

take 1 and maybe we can use a file top level or module config for these files

[JarryShaw]

Ruling: option 1. The headings become plain # Header Schemas / # Data Models. Instead of a marker on each heading, these __init__s are protected at the file or module level.

Round 4 picks one mechanism. A repo [tool.isort] config in pyproject.toml (extend_skip_glob for pcapkit/protocols/**/__init__.py, plus filter_files so a direct isort <file> also skips them) is preferred if it holds for both make isort and a direct run. If it does not, the fallback is one # isort: skip_file line at the top of each file. The repo has no [tool.isort] yet. PackageInitLayoutTests keeps guarding the order either way.

[JarryShaw]

Round 4 pushed: 9efd56dfc → ac734d8c6, a fresh codemod run on main 4f62f52ad with no hand edits.

• Headings are now exactly # Header Schemas / # Data Models, and no isort: split is left anywhere.
• A new [tool.isort] in pyproject.toml (extend_skip_glob for the protocol package __init__.pys, plus filter_files) means a direct isort run on one of them, or on all of them, reports it skipped. Without the section, the same run rewrites ipv4's. make isort stays clean.
• PackageInitLayoutTests fails if a heading regains the marker, if the blocks swap, or if the config section is removed.
• All legs, guards, isort, docs and the shipped-names check pass. A short cross-review of the delta is running.

[JarryShaw]

GOOD TO GO at ac734d8c6, from a Sonnet review of round 4.

• Since round 3, the only changes are the plain headings, the [tool.isort] section, the conventions example, PackageInitLayoutTests and a tier-guard doc line. Nothing else under pcapkit/ changed.
• A direct isort run skips the package __init__s, from the repo root or a subdirectory. Without the section, isort rewrites ipv4's. The glob matches the protocol packages plus the three misc packages that use role blocks, and nothing else. make isort is unchanged.
• Putting the marker back, swapping the blocks or removing the section each fails the layout test.
• Project tests, Lint and deploy-pages pass.