[JarryShaw]

• Searched for similar pull requests
• Followed the coding style
• make test passes, and a test case covers the change
• Changelog entry: N/A — centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

• fix — bug fix

---

Description

Closes #2285.

• Fix: FTP_DATA now subclasses Application as well as Raw (class FTP_DATA(Application, Raw)), so it has an __layer__ and layer='application'/layer=FTP_DATA extraction stops at it.
• One behaviour change: __index__ now raises IntError (a TypeError) rather than UnsupportedCall, matching FTP. Noted with a versionchanged.
• Moved to its own module: FTP and FTP_DATA are reached under different AppType/port entries (unlike RARP/DRARP, which share one registry code), so FTP_DATA moves out of ftp.py into a new ftp_data.py rather than staying a sibling there. It needed nothing else ftp.py imports — a pure move. The old path keeps nothing behind it (refactor!: remove every surviving re-export of a moved name or module #1744's no-re-exports rule); a versionremoved note on ftp.py and its docs page records where it shipped.
• Fail-without-fix: 5 failed on origin/main. Passing on the rebased head (53 passed, 229 subtests).

[JarryShaw]

Review: NEEDS CHANGES at 4bd130c69 (Sonnet) — one change-relative comment, code is correct.

• Confirmed: FTP_DATA(Application, Raw) gives it a real __layer__, so layer extraction stops at it; parse/make/from_data still resolve to Raw's implementation (Application's short-read wrappers are inert here); __index__ now raises IntError with an accurate versionchanged; no dispatch or registry code is affected. Fail-without-fix: 5 failed on origin/main. On the head, 564+354 passed; reversing the base order breaks 2 tests, confirming it's pinned.
• Required: reword foreign()'s new docstring in test_all_layer_grouping_unit.py — it reads as change-relative history ("reported None ... until it gained"), which the docs: #719-style prose sweep after the #1090–#1102 defect wave #1103 sweep removes.
• Non-blocking: the versionchanged note could name __layer__ and .layer explicitly; a redundant pylint disable comment; the __index__ assertion could be its own test.

[JarryShaw]

Amended 4bd130c69 → ee70b4f82: reworded foreign()'s docstring in test_all_layer_grouping_unit.py to state the behaviour rather than its history. test_all_layer_grouping_unit.py + test_layer_placement_unit.py + test_docstring_contract.py give 33 passed.

[github-actions]

Coverage: 91.68% (unit tier, Python 3.15, 800a6ec4b, Unit Tests run success)

Package	Statements	Missed	Branches	Partial	Cover
pcapkit (top level)	104	4	20	2	95.16%
pcapkit/const	19601	880	2620	867	92.12%
pcapkit/corekit	2596	74	818	37	96.51%
pcapkit/dumpkit	282	3	102	3	98.44%
pcapkit/foundation	3044	23	1060	25	98.83%
pcapkit/interface	138	4	52	3	96.32%
pcapkit/protocols	17486	209	4602	178	98.24%
pcapkit/toolkit	914	36	334	10	95.51%
pcapkit/utilities	502	7	144	5	98.14%
pcapkit/vendor	4434	1961	1018	167	52.49%

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

[JarryShaw]

Ready to merge at ee70b4f82. Closes #2285.

• Review: NEEDS CHANGES → fixed (change-relative test docstring) → self-certified, prose-only change.
• CI: Required checks passed SUCCESS, 0 failures, KTLO clean.
• Ordering leg: protocols gave 1971 tests with 0 failures, run with the PR merged into current main.
• Merge state: mergeable.

[JarryShaw]

should we move FTP_DATA to its own file then? now that is a full first class protocol?

[JarryShaw]

My recommendation: keep it in ftp.py. There's a direct precedent for two first-class, independently-dispatchable protocols sharing a file when they're this tightly coupled — rarp.py already pairs RARP and DRARP (a variant of it) in one file, same as httpv1.py pairs HTTP with its Type enum. FTP/FTP_DATA are the control and data channels of the same protocol family, which reads as at least as coupled as RARP/DRARP. Splitting would also mean duplicating or cross-importing the module's shared Type enum. Happy to split it out if you'd rather establish one-class-per-file going forward, but I don't see a strong case for it here specifically.

[JarryShaw]

Correction to my last comment: checked closer, and the precedent doesn't actually hold. RARP/DRARP share one file because they share one dispatch destination — both resolve __index__ to the same Enum_EtherType member, so it's one registry slot with two implementations, not two independently-dispatched protocols. FTP/FTP_DATA don't share a registry code at all (neither registers a dispatch index in ftp.py), so that precedent doesn't transfer.

Without it, I don't have a clean house-convention answer either way — this is a genuine judgment call rather than something existing code settles. Your call on whether to split it out.

[JarryShaw]

iirc, FTP and FTP_DATA are registered under different AppType/port. so better still separate.

[JarryShaw]

Splitting FTP_DATA into its own file (ftp_data.py) per your ruling — different AppType/port entries, so independently dispatched. Will post the diff shortly.

[JarryShaw]

Review: GOOD TO GO at 800a6ec4b (Sonnet). ftp_data.py needs only Application/Raw/Literal, 0 undefined/unused names. No re-export survives (hasattr(ftp, 'FTP_DATA') is False, absent from __all__, raises ImportError from the old path); mutation-tested both ledger tests (re-adding a re-export, reverting the #1739 key, and reverting to class FTP_DATA(Raw) all fail the right tests). 281 passed/5 failed matches base exactly (the 5 need generated captures, same on base). Docs built clean in the pinned Sphinx venv, ext.rst table aligned, no stale references. __index__'s IntError vs UnsupportedCall change verified real and documented via versionchanged. mypy/pylint identical to base; ftp_data.py is 100% covered. One non-blocking nit: the copied # pylint: disable=abstract-method on FTP_DATA is unneeded under pylint 4.0.8.