Repository navigation
docs(foundation): cite the real class statement in the engine-check note - #763
Merged
Merged
Conversation
- `Extractor.register_engine`'s `# NOTE:` explained its `issubclass(engine, EngineBase)` gate by citing `engines/pcap.py` importing `EngineBase as Engine`. #750 removed that alias: the file now imports `EngineBase` under its own name and declares `class PCAP(EngineBase[Frame])`, so the parenthetical pointed a reader at a shape that is not there. - Cites the class statement rather than a line number, a line citation being the same staleness class. The conclusion is unchanged and still correct: the built-ins subclass the base directly to decline `Engine.__init_subclass__`'s auto-registration, which is why the gate is wide. `See #513.` kept. Comment-only. Verified no `as Engine` survives anywhere under `pcapkit/`, and that the file's AST is identical to `origin/main`'s (434 statements both sides), so coverage is unchanged. tests/foundation 259 passed / 11 skipped / 404 subtests passed, plus tests/test_base_class_contract.py. Fixes #756.
JarryShaw
added
docs
Owner
Author
|
Cross-review verdict: GOOD TO GO (sonnet; authored by a different model). All four clauses of the new comment verified true, |
JarryShaw
removed
the
review: good-to-go
JarryShaw
added a commit
that referenced
this pull request
Sep 26, 2026
`main` moved from 73f09ae to 3cbdf89 while this PR sat open. This commit (relative to its parent, 4233555) now names all 20 bullets it carries, not just the 8 this session added on top of the 10 already there -- a first draft of this message named only its own 8 and left the other 10 silent, which a cross-review caught. Four are breaking: - #754 -- AppType split into per-transport registries; the 1,004 portless and 704 transportless rows stop being members. - #764 -- an out-of-range port in `AppType.get` is refused, not minted. - #778 -- `@final` enforced at runtime on `Info`/`Schema`. - #575 -- four `.get()`-backed enum fields fall through to `_unregistered_member` instead of minting; 14 of 23 sample captures change output. - #772 (with #790's docstring reword), #766, #759, #787, #794 (with #791's citation repoint), #792/#798, #802, #782 (a further #745-hazard instance), #704, #723, #739, #743/#746 (cross-dependent, one bullet each), #805 (closes #802's own filed-as-out-of-scope), #796, #800 -- the other 16, non-breaking. Also restores a measurement an earlier round in this same diff dropped while updating an adjacent one: the #692 entry's "mypy is unmoved at 112 errors" silently lost its "and pylint ... 364 messages" half when `EXPECTED_FAILURES` was corrected 44 to 43 elsewhere in the same sentence. Restored to the last value earlier rounds signed off on rather than re-measured, since this branch's own `pcapkit/` tree predates several since-merged PRs and a fresh run would not be measuring the same thing the original round measured. Unmoved, and not silently dropped this time: `93 of the 95 sites` corrected to `94 of the 95` and `EXPECTED_FAILURES` 44 to 43 in the several other places that already carried the fix. On which PRs get a bullet: there is no clean "user-facing only" rule -- #766 and #791 are pure CI/test/lint-comment entries that are in, while #773 and #763 are the same kind of thing and are out. The real pattern across this file's 46 commits is closer to "each round's author judged it worth a reader's time," which is inconsistent by construction. This round leaves that inconsistency as found rather than trying to retrofit a rule, but did add #782 on reconsideration -- its own PR body names it as sharing #766's hazard, and pre-existing precedent already treats that hazard's instances as bullet-worthy. `changelog_md.py` regenerated `CHANGELOG.md`; `--check` exit 0. `test_changelog_md.py` 47 passed.
JarryShaw
added a commit
that referenced
this pull request
Sep 26, 2026
`main` moved from 73f09ae to 3cbdf89 while this PR sat open. This commit (relative to its parent, 4233555) now names all 20 bullets it carries, not just the 8 this session added on top of the 10 already there -- a first draft of this message named only its own 8 and left the other 10 silent, which a cross-review caught. Six are breaking, matching the crediting PRs' own `breaking` label in each case: - #754 -- AppType split into per-transport registries; the 1,004 portless and 704 transportless rows stop being members. - #764 -- an out-of-range port in `AppType.get` is refused, not minted. - #778 -- `@final` enforced at runtime on `Info`/`Schema`. - #575 -- four `.get()`-backed enum fields fall through to `_unregistered_member` instead of minting; 14 of 23 sample captures change output. - #759 -- `AppType._dispatch` on a multi-transport `proto` now raises `ProtocolError` instead of silently resolving to whichever transport owns the lowest set bit. - #805 -- `FieldBase.length` on a negative resolved length now raises `ProtocolError` instead of letting a bare `struct.error` escape. A second cross-review caught both: their crediting PRs (#783, #811) both carry GitHub's own `breaking` label, and neither bullet said so. - #772 (with #790's docstring reword), #766, #787, #794 (with #791's citation repoint), #792/#798, #802, #779 (via #782, a further #745-hazard instance), #704, #723, #739, #743/#746 (cross-dependent, one bullet each), #796, #800 -- the other 14, non-breaking. Also restores a measurement an earlier round in this same diff dropped while updating an adjacent one: the #692 entry's "mypy is unmoved at 112 errors" silently lost its "and pylint ... 364 messages" half when `EXPECTED_FAILURES` was corrected 44 to 43 elsewhere in the same sentence. Restored to the last value earlier rounds signed off on rather than re-measured, since this branch's own `pcapkit/` tree predates several since-merged PRs and a fresh run would not be measuring the same thing the original round measured. Unmoved, and not silently dropped this time: `93 of the 95 sites` corrected to `94 of the 95` and `EXPECTED_FAILURES` 44 to 43 in the several other places that already carried the fix. On which PRs get a bullet: there is no clean "user-facing only" rule -- #766 and #791 are pure CI/test/lint-comment entries that are in, while #773 and #763 are the same kind of thing and are out. The real pattern across this file's 46 commits is closer to "each round's author judged it worth a reader's time," which is inconsistent by construction. This round leaves that inconsistency as found rather than trying to retrofit a rule, but did add #779 (via #782) on reconsideration -- its own PR body names it as sharing #766's hazard, and pre-existing precedent already treats that hazard's instances as bullet-worthy. `changelog_md.py` regenerated `CHANGELOG.md`; `--check` exit 0. `test_changelog_md.py` 47 passed.
JarryShaw
added a commit
that referenced
this pull request
Sep 26, 2026
`main` moved from 73f09ae to 3cbdf89 while this PR sat open. This commit (relative to its parent, 4233555) now names all 20 bullets it carries, not just the 8 this session added on top of the 10 already there -- a first draft of this message named only its own 8 and left the other 10 silent, which a cross-review caught. Six are breaking, matching the crediting PRs' own `breaking` label in each case: - #754 -- AppType split into per-transport registries; the 1,004 portless and 704 transportless rows stop being members. - #764 -- an out-of-range port in `AppType.get` is refused, not minted. - #778 -- `@final` enforced at runtime on `Info`/`Schema`. - #575 -- four `.get()`-backed enum fields fall through to `_unregistered_member` instead of minting; 14 of 23 sample captures change output. - #759 -- `AppType._dispatch` on a multi-transport `proto` now raises `ProtocolError` instead of silently resolving to whichever transport owns the lowest set bit. - #805 -- `FieldBase.length` on a negative resolved length now raises `ProtocolError` instead of letting a bare `struct.error` escape. A second cross-review caught both: their crediting PRs (#783, #811) both carry GitHub's own `breaking` label, and neither bullet said so. - #772 (with #790's docstring reword), #766, #787, #794 (with #791's citation repoint), #792/#798, #802, #779 (via #782, a further #745-hazard instance), #704, #723, #739, #743/#746 (cross-dependent, one bullet each), #796, #800 -- the other 14, non-breaking. Also restores a measurement an earlier round in this same diff dropped while updating an adjacent one: the #692 entry's "mypy is unmoved at 112 errors" silently lost its "and pylint ... 364 messages" half when `EXPECTED_FAILURES` was corrected 44 to 43 elsewhere in the same sentence. Restored to the last value earlier rounds signed off on rather than re-measured, since this branch's own `pcapkit/` tree predates several since-merged PRs and a fresh run would not be measuring the same thing the original round measured. Unmoved, and not silently dropped this time: `93 of the 95 sites` corrected to `94 of the 95` and `EXPECTED_FAILURES` 44 to 43 in the several other places that already carried the fix. On which PRs get a bullet: there is no clean "user-facing only" rule -- #766 and #791 are pure CI/test/lint-comment entries that are in, while #773 and #763 are the same kind of thing and are out. The real pattern across this file's 46 commits is closer to "each round's author judged it worth a reader's time," which is inconsistent by construction. This round leaves that inconsistency as found rather than trying to retrofit a rule, but did add #779 (via #782) on reconsideration -- its own PR body names it as sharing #766's hazard, and pre-existing precedent already treats that hazard's instances as bullet-worthy. `changelog_md.py` regenerated `CHANGELOG.md`; `--check` exit 0. `test_changelog_md.py` 47 passed.
JarryShaw
added a commit
that referenced
this pull request
Sep 26, 2026
`main` moved from 73f09ae to 3cbdf89 while this PR sat open. This commit (relative to its parent, 4233555) now names all 20 bullets it carries, not just the 8 this session added on top of the 10 already there -- a first draft of this message named only its own 8 and left the other 10 silent, which a cross-review caught. Six are breaking, matching the crediting PRs' own `breaking` label in each case: - #754 -- AppType split into per-transport registries; the 1,004 portless and 704 transportless rows stop being members. - #764 -- an out-of-range port in `AppType.get` is refused, not minted. - #778 -- `@final` enforced at runtime on `Info`/`Schema`. - #575 -- four `.get()`-backed enum fields fall through to `_unregistered_member` instead of minting; 14 of 23 sample captures change output. - #759 -- `AppType._dispatch` on a multi-transport `proto` now raises `ProtocolError` instead of silently resolving to whichever transport owns the lowest set bit. - #805 -- `FieldBase.length` on a negative resolved length now raises `ProtocolError` instead of letting a bare `struct.error` escape. A second cross-review caught both: their crediting PRs (#783, #811) both carry GitHub's own `breaking` label, and neither bullet said so. - #772 (with #790's docstring reword), #766, #787, #794 (with #791's citation repoint), #792/#798, #802, #779 (via #782, a further #745-hazard instance), #704, #723, #739, #743/#746 (cross-dependent, one bullet each), #796, #800 -- the other 14, non-breaking. Also restores a measurement an earlier round in this same diff dropped while updating an adjacent one: the #692 entry's "mypy is unmoved at 112 errors" silently lost its "and pylint ... 364 messages" half when `EXPECTED_FAILURES` was corrected 44 to 43 elsewhere in the same sentence. Restored to the last value earlier rounds signed off on rather than re-measured, since this branch's own `pcapkit/` tree predates several since-merged PRs and a fresh run would not be measuring the same thing the original round measured. Unmoved, and not silently dropped this time: `93 of the 95 sites` corrected to `94 of the 95` and `EXPECTED_FAILURES` 44 to 43 in the several other places that already carried the fix. On which PRs get a bullet: there is no clean "user-facing only" rule -- #766 and #791 are pure CI/test/lint-comment entries that are in, while #773 and #763 are the same kind of thing and are out. The real pattern across this file's 46 commits is closer to "each round's author judged it worth a reader's time," which is inconsistent by construction. This round leaves that inconsistency as found rather than trying to retrofit a rule, but did add #779 (via #782) on reconsideration -- its own PR body names it as sharing #766's hazard, and pre-existing precedent already treats that hazard's instances as bullet-worthy. `changelog_md.py` regenerated `CHANGELOG.md`; `--check` exit 0. `test_changelog_md.py` 47 passed.
JarryShaw
added a commit
that referenced
this pull request
Sep 28, 2026
`main` moved from 73f09ae to 3cbdf89 while this PR sat open. This commit (relative to its parent, 4233555) now names all 20 bullets it carries, not just the 8 this session added on top of the 10 already there -- a first draft of this message named only its own 8 and left the other 10 silent, which a cross-review caught. Six are breaking, matching the crediting PRs' own `breaking` label in each case: - #754 -- AppType split into per-transport registries; the 1,004 portless and 704 transportless rows stop being members. - #764 -- an out-of-range port in `AppType.get` is refused, not minted. - #778 -- `@final` enforced at runtime on `Info`/`Schema`. - #575 -- four `.get()`-backed enum fields fall through to `_unregistered_member` instead of minting; 14 of 23 sample captures change output. - #759 -- `AppType._dispatch` on a multi-transport `proto` now raises `ProtocolError` instead of silently resolving to whichever transport owns the lowest set bit. - #805 -- `FieldBase.length` on a negative resolved length now raises `ProtocolError` instead of letting a bare `struct.error` escape. A second cross-review caught both: their crediting PRs (#783, #811) both carry GitHub's own `breaking` label, and neither bullet said so. - #772 (with #790's docstring reword), #766, #787, #794 (with #791's citation repoint), #792/#798, #802, #779 (via #782, a further #745-hazard instance), #704, #723, #739, #743/#746 (cross-dependent, one bullet each), #796, #800 -- the other 14, non-breaking. Also restores a measurement an earlier round in this same diff dropped while updating an adjacent one: the #692 entry's "mypy is unmoved at 112 errors" silently lost its "and pylint ... 364 messages" half when `EXPECTED_FAILURES` was corrected 44 to 43 elsewhere in the same sentence. Restored to the last value earlier rounds signed off on rather than re-measured, since this branch's own `pcapkit/` tree predates several since-merged PRs and a fresh run would not be measuring the same thing the original round measured. Unmoved, and not silently dropped this time: `93 of the 95 sites` corrected to `94 of the 95` and `EXPECTED_FAILURES` 44 to 43 in the several other places that already carried the fix. On which PRs get a bullet: there is no clean "user-facing only" rule -- #766 and #791 are pure CI/test/lint-comment entries that are in, while #773 and #763 are the same kind of thing and are out. The real pattern across this file's 46 commits is closer to "each round's author judged it worth a reader's time," which is inconsistent by construction. This round leaves that inconsistency as found rather than trying to retrofit a rule, but did add #779 (via #782) on reconsideration -- its own PR body names it as sharing #766's hazard, and pre-existing precedent already treats that hazard's instances as bullet-worthy. `changelog_md.py` regenerated `CHANGELOG.md`; `--check` exit 0. `test_changelog_md.py` 47 passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
make pylint,make mypy,make isort)make testpasses, and a test case covers the changedocs/source/changelog/and regeneratedCHANGELOG.md, if the change is user-visible — 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?
fix— corrects a defectfeat— adds a featureperf— changes performance, not behaviourrefactor— changes neither behaviour nor performancetest— tests onlydocs— documentation onlyci— workflows or build toolingchore— anything elseDescription of your pull request and other information
Fixes #756.
Extractor.register_engine's# NOTE:explained itsissubclass(engine, EngineBase)gate by citing an alias #750 removed. Verified: noEngineBase as Engine— noas Engineat all — survives anywhere underpcapkit/.The one substantive change is the parenthetical; the other four lines are rewrapping.
engines/pcap.pyimports it asEngineBase as Engineengines/pcap.pydeclaresclass PCAP(EngineBase[Frame])The conclusion is untouched and still correct, and
See #513.is kept. The class statement is cited rather than a line number, a line citation being the same staleness class. The sibling gates inregister_reassembly/register_traceflowdefer to this note, so it is the one canonical explanation.One sibling left unfixed, deliberately.
tests/foundation/registry/test_foundation.py:118-119's docstring cites the same removed alias atengines/pcap.py. Out of this PR's scope — happy to take it in a follow-up. The survivingFieldBase as Fieldsites are live imports rather than stale prose;tests/test_base_class_contract.py:101exemptsFieldBaseby name.Coverage is flat, and provable rather than waived. The file's AST is identical to
origin/main's — 434 statements both sides — so not one statement is added or removed. Nothing in the suite can catch a stale comment:test_base_class_contract.pywalksast.Import/ast.ImportFrom, to which comments are invisible. A grep-based prose check overpcapkit/for*Base as *outside an import would have caught this one.Tests (
.venv3.14.7,PYTHONSAFEPATH=1,pcapkit.__file__asserted inside the worktree).tests/foundation259 passed, 11 skipped, 404 subtests passed;tests/foundation/registry/test_foundation.py+tests/test_base_class_contract.pyundercoverage run10 passed, 31 subtests passed.test_tcp_runtime.pyfailed untilexamples/generators/make_samples.pywas run.isort -l100 -ppcapkitclean; pylint C0301 count identical toorigin/main(30 on both).