Repository navigation
fix(const): refuse a multi-bit proto in AppType._dispatch rather than resolving it LSB-first (#759) - #783
Conversation
JarryShaw
added
fix
|
Cross-review at 1. The "12,341 declared single-bit ports" figure is wrong, and I re-derived it myself rather than take the reviewer's word. Measured on So the single-bit set is 1,766, not 12,341 — and 12,341 is 50 short of the total 12,391, not of the single-bit subset, which suggests the figure was a total with something excluded and then mislabelled. That makes digest 2. 3. The changelog checkbox is unticked with no annotation. House form is One overstatement worth correcting while you are in there. The body says only the UDP half of the 46 is #759's cross-registry dispatch. Measured: of the 23 UDP-origin mismatches, only 2 (ports 888 and 999) actually return a different final service than UDP's own single-bit lookup would; the other 21 land on TCP mechanically but coincide with UDP's canonical string, exactly like the TCP-origin cases. The 23/23 split is right, the characterisation of what each half means is not. Verified good, independently derived. The ten call sites are confirmed by direct read before the reviewer consulted your AST test — Not verified by the reviewer: the live-IANA byte-identical regeneration, and the coverage/pylint deltas — out of its budget, not disputed. |
JarryShaw
added
review: needs-changes
8261139 to
515a826
Compare
JarryShaw
added
review: pending
|
All three corrected at So 12,341 is the distinct The blast radius is now stated honestly, and pinned. Of the 46: 44 are
Two things done right that I did not ask for. You re-fetched IANA before regenerating, confirmed the CSV byte-identical to your baseline ( Label back to |
|
Round-two cross-review at All three required changes confirmed applied, and the reviewer independently recomputed the four counts rather than accepting either my figures or the author's: 12,391 members / 10,625 multi-bit / 1,766 single-bit / 12,341 distinct The const-file delta really is comment-only. Diffing The divergent pin has teeth. One honest limit worth recording. The reviewer could not reproduce the exact digest hex The merge check was redone rather than trusted. Merged Settled in round one and not re-run: the ten call sites, Six CI legs still outstanding (21 success, 3 expected skips, 0 failures), so this is a review verdict rather than a merge signal. Host load ran 3.08 → 4.84 during the review from other agents, so it was deliberately kept to the requested checks. |
JarryShaw
added
review: good-to-go
| # ignored, so nothing there is ambiguous to begin with. | ||
| namespaces = show_flag_values(proto) | ||
| if len(namespaces) > 1: | ||
| raise ProtocolError('%r names %d transport protocols, and so %d registries of %s; ' |
There was a problem hiding this comment.
Because this region is a str.format() template in the vendor generator, not hand-written source. pcapkit/vendor/reg/apptype/apptype.py emits the const file through .format() — you can see the delimiters in the same template at :243, __registries__: 'dict[TransportProtocol, Type[{NAME}]]' = {{}}, where the empty dict has to be written {{}} to survive expansion.
So emitting an f-string means writing f'{{proto!r}} names {{len(namespaces)}} …' in the template: every brace doubled, and the generator line stops reading like the code it produces.
Measured, so the local-versus-global convention is explicit:
| scope | raise …Error(f'… |
raise …Error('…' % …) |
|---|---|---|
pcapkit/ whole tree |
605 | 2 |
const/reg/apptype/apptype.py on origin/main |
0 | 6 |
So the repo convention really is f-strings, and this generated file is the exception — all six of its pre-existing raises are %-style (:92, :2308, :2368, :2378, :2478, :2484), and the new one matched them rather than being the only f-string in the file.
Happy to switch it if you would rather the emitted code follow the tree convention — it is one raise, so four doubled braces in the template. Say which and I will make it match.
There was a problem hiding this comment.
yea, id like to keep f-string convention across the library. only use % substitution when inevitable.
There was a problem hiding this comment.
Recorded, in your words: "yea, id like to keep f-string convention across the library. only use % substitution when inevitable."
It is not inevitable here — the doubled-brace form survives the generator's .format() intact. Verified before routing it, template in and expanded code out:
raise ProtocolError(f'{{proto!r}} names {{len(namespaces)}} transport protocols, and so ' <- in the template
f'{{len(namespaces)}} registries of {{cls.__name__}}; look the port up '
'under one transport protocol at a time')
raise ProtocolError(f'{proto!r} names {len(namespaces)} transport protocols, and so ' <- emitted
f'{len(namespaces)} registries of {cls.__name__}; look the port up '
'under one transport protocol at a time')
expanded raise parses as valid Python: True
So the new raise converts, alongside the loop flattening, in the same amend.
Scoping the other six deliberately, rather than quietly. This file has 6 pre-existing %-style raises — :92, :2308, :2368, :2378, :2478, :2484 — all of which your convention also wants converted. I am leaving them out of #783 and filing them as their own issue, because #783 is the multi-bit-proto refusal and a const diff that also sweeps style across a 12,391-member generated file is much harder to review for the thing it is actually doing.
Say the word if you would rather have all seven in one pass here and I will fold them in instead.
There was a problem hiding this comment.
Correcting myself on one detail above. I said this region is a str.format() template. It is not — BASE at pcapkit/vendor/reg/apptype/apptype.py:93 is a lambda returning an f-string:
BASE = lambda NAME, DOCS, FLAG, TABLE, MISS, MODL: f'''\
The brace-doubling conclusion is unchanged, since an f-string escapes braces the same way — and :243's = {{}} is still the evidence for it. But anyone following my earlier wording would go looking for a .format() call that is not there. The generated raise and the flattening are unaffected.
| raise ProtocolError('%r names %d transport protocols, and so %d registries of %s; ' | ||
| 'look the port up under one transport protocol at a time' | ||
| % (proto, len(namespaces), len(namespaces), cls.__name__)) | ||
| for namespace in namespaces: |
There was a problem hiding this comment.
if already raised on multiple namespaces, then why the loop on one element list?
There was a problem hiding this comment.
You are right, and it is a real simplification — the loop cannot iterate more than once.
After the len(namespaces) > 1 raise directly above it, namespaces is length 0 or 1, so the for is an if wearing a for. Both surviving lengths measured:
undefined int=0 show_flag_values=[] len=0
tcp int=1 show_flag_values=[1] len=1
tcp|udp int=3 show_flag_values=[1, 2] len=2 <- raises above
The 0-length case is live, not theoretical — it is the default argument reaching the delegating path:
AppType.get(80, proto=undefined)
-> ValueError: <TransportProtocol.undefined: 0> names no transport protocol registry of AppType
so the fall-through to the trailing ValueError has to stay. What the loop was doing was making a one-or-zero case look like an n-case, which reads as if a composite could still be resolved here — the opposite of what the raise above it decided.
Flattening to:
if namespaces:
subclass = cls.__registries__.get(TransportProtocol(namespaces[0]))
if subclass is not None:
return subclass
raise ValueError(...)Applying that to the generator template and regenerating, then re-posting the revision. Flipping to review: needs-changes until it lands.
JarryShaw
added
review: needs-changes
515a826 to
7caec43
Compare
… resolving it LSB-first (#759) `AppType._dispatch` resolved a `proto` naming several transport protocols by taking its lowest set bit: `show_flag_values` iterates LSB-first and `tcp` is the lowest declared bit, so every composite containing it dispatched into the TCP registry whatever else it named. `TransportProtocol` is an `aenum.IntFlag` and a member carries the whole set IANA assigned the service, so `tcp | udp` is an ordinary value to read off one and an ordinary thing to pass back in. Re-measured on `fe80b8525`, sweeping all 10,625 multi-transport members through `get(m.port, proto=m.proto)`: 0 exceptions, 0 mints, and 46 answers naming a service other than the member's own, at 20 distinct ports. Those 46 are not all defects. 44 differ only in that the member is not its port's canonical, which a single-bit lookup does too and which `get` documents; 23 -- the UDP-declared half -- came back as a member of the *TCP* registry, so the type and `proto` were wrong whatever the service string said; and 2 named a service UDP does not answer for the port at all. Those two are the whole blast radius: port 888, where `accessbuilder` resolved to TCP's `cddbp`, and port 999, where `puprouter` resolved to TCP's `garcon` against UDP's `applix`. All of it silent, and undetectable to a caller checking equality, since `AppType.__eq__` compares on `port` alone. Identical counts at `83b58ebda` and `932cb48d1`, so long-standing rather than a regression. It now raises `ProtocolError` from `pcapkit.utilities.exceptions` -- a `ValueError` subclass, so the documented contract of `get` and `get_all` still holds, and a loud error rather than a quiet one because nothing catches it. Refusing costs no caller: the ten library call sites into the lookup each name one transport protocol, and `register_apptype`, the one place that reads a member's composite `proto`, tests it with `in` and never reaches the lookup. The change is in the `BASE` template of `pcapkit/vendor/reg/apptype/apptype.py`, which generates `pcapkit/const/reg/apptype/apptype.py`. The const file's matching edit was applied by hand rather than by a crawl, because regenerating fetches IANA's live registry and would rewrite unrelated member rows; expanding `BASE` and diffing the `_dispatch` region against the const file gives 78 lines each and 0 diff lines, so it is byte-identical to what regeneration emits. `tcp.py`, `udp.py`, `sctp.py` and `dccp.py` are untouched.
7caec43 to
9aada6a
Compare
|
Delta cross-review at What it re-derived independently, not accepted:
The refutation, and it was right. My brief said the branch sits on top of current One thing it flagged worth keeping: the Setting |
JarryShaw
added
review: good-to-go
JarryShaw
removed
the
review: good-to-go
) The generated `pcapkit.const.reg.apptype.apptype` module had 6 `raise ValueError(...)` calls still using `%` formatting, left that way by #783 so its own diff over a 12,391-member file stayed reviewable. Per the maintainer's f-string convention (#783), they now match the other 605 f-string raises tree-wide: - `TransportProtocol._missing_`: `'%r is not a valid %s' % (...)` -> `f'{value!r} is not a valid {cls.__name__}'` - `AppType.__new__`: the "holds no members" guard - `AppType._dispatch`: the non-port `key` guard and the trailing "names no transport protocol registry" guard - `AppType._missing_`: both range/registry guards (same message as the first bullet) Each message was verified byte-identical old vs new (same repr()/str() semantics for %r/!r and %s/{}), and exercised through its real call path (`AppType.get('http', proto=tcp)`, `AppType.get(80, proto=undefined)`, `extend_enum(AppType, ...)`, out-of-range `AppType(...)`) with matching live exception text before and after. The edit is in the `BASE` template of `pcapkit/vendor/reg/apptype/apptype.py` (braces doubled, as an f-string template emitting f-strings) and applied by hand to the const file rather than by a crawl, to avoid rewriting the IANA-derived member rows. Expanding `BASE` with dummy TABLE/MISS and diffing against the const file shows exactly 2 non-equal regions -- the docstring's list-table and the range-row tail of `_missing_`, both TABLE/MISS-dependent -- and 0 elsewhere. Left unconverted, and said why in the PR: the three %-formatted dunders (`__new__`, `__repr__`, `__str__`) the issue also mentions -- out of the issue's stated raise-only scope, and `__new__`'s format string sets every member's underlying `StrEnum` value across all ~12,391 real members, a much larger blast radius than an error path. The module-level `consider-using-f-string` pylint disable stays: those dunders and the `extend_enum(..., '%d' % value, ...)` calls still use `%`. `tests/const/test_const_enum_builtin_parity.py::test_every_bespoke_template_ carries_the_guard` (GitHub issue #647) pinned all four bespoke vendor templates' guards against one shared `%`-style literal, so converting `reg.apptype.apptype`'s copy broke it in CI (7 legs). `BESPOKE_TEMPLATES` is now a dict keyed to each template's own guard text -- the other three still raise with `%` (#796 tracks sweeping them) -- rather than an `or` of both forms, which would let a genuinely deleted guard pass. Proved by deleting the guard from `pcapkit.vendor.tcp.flags` locally: the test failed with the expected subTest and message, then the file was restored and the tree verified clean. Build/test: `tests/const/test_const_apptype_split_unit.py` (19), `tests/vendor/test_vendor_reg_apptype_generator_unit.py` (6), and `tests/const/test_const_enum_builtin_parity.py` (20, `requests` importable so neither guard test skips) all pass under both pytest and plain unittest. mypy clean before and after. pylint: 0 new findings in either touched apptype file; R0801 unchanged at 575 tree-wide. Closes #792
) The generated `pcapkit.const.reg.apptype.apptype` module had 6 `raise ValueError(...)` calls still using `%` formatting, left that way by #783 so its own diff over a 12,391-member file stayed reviewable. Per the maintainer's f-string convention (#783), they now match the other 605 f-string raises tree-wide: - `TransportProtocol._missing_`: `'%r is not a valid %s' % (...)` -> `f'{value!r} is not a valid {cls.__name__}'` - `AppType.__new__`: the "holds no members" guard - `AppType._dispatch`: the non-port `key` guard and the trailing "names no transport protocol registry" guard - `AppType._missing_`: both range/registry guards (same message as the first bullet) Each message was verified byte-identical old vs new (same repr()/str() semantics for %r/!r and %s/{}), and exercised through its real call path (`AppType.get('http', proto=tcp)`, `AppType.get(80, proto=undefined)`, `extend_enum(AppType, ...)`, out-of-range `AppType(...)`) with matching live exception text before and after. The edit is in the `BASE` template of `pcapkit/vendor/reg/apptype/apptype.py` (braces doubled, as an f-string template emitting f-strings) and applied by hand to the const file rather than by a crawl, to avoid rewriting the IANA-derived member rows. Expanding `BASE` with dummy TABLE/MISS and diffing against the const file shows exactly 2 non-equal regions -- the docstring's list-table and the range-row tail of `_missing_`, both TABLE/MISS-dependent -- and 0 elsewhere. Left unconverted, and said why in the PR: the three %-formatted dunders (`__new__`, `__repr__`, `__str__`) the issue also mentions -- out of the issue's stated raise-only scope, and `__new__`'s format string sets every member's underlying `StrEnum` value across all ~12,391 real members, a much larger blast radius than an error path. The module-level `consider-using-f-string` pylint disable stays: those dunders and the `extend_enum(..., '%d' % value, ...)` calls still use `%`. `tests/const/test_const_enum_builtin_parity.py::test_every_bespoke_template_ carries_the_guard` (GitHub issue #647) pinned all four bespoke vendor templates' guards against one shared `%`-style literal, so converting `reg.apptype.apptype`'s copy broke it in CI (7 legs). `BESPOKE_TEMPLATES` is now a dict keyed to each template's own guard text -- the other three still raise with `%` (#798 tracks sweeping them, the `%`-formatted dunders, and dropping the disable) -- rather than an `or` of both forms, which would let a genuinely deleted guard pass. Proved by deleting the guard from `pcapkit.vendor.tcp.flags` locally: the test failed with the expected subTest and message, then the file was restored and the tree verified clean. Build/test: `tests/const/test_const_apptype_split_unit.py` (19), `tests/vendor/test_vendor_reg_apptype_generator_unit.py` (6), and `tests/const/test_const_enum_builtin_parity.py` (20, `requests` importable so neither guard test skips) all pass under both pytest and plain unittest. mypy clean before and after. pylint: 0 new findings in either touched apptype file; R0801 unchanged at 575 tree-wide. Closes #792
) (#797) The generated `pcapkit.const.reg.apptype.apptype` module had 6 `raise ValueError(...)` calls still using `%` formatting, left that way by #783 so its own diff over a 12,391-member file stayed reviewable. Per the maintainer's f-string convention (#783), they now match the other 605 f-string raises tree-wide: - `TransportProtocol._missing_`: `'%r is not a valid %s' % (...)` -> `f'{value!r} is not a valid {cls.__name__}'` - `AppType.__new__`: the "holds no members" guard - `AppType._dispatch`: the non-port `key` guard and the trailing "names no transport protocol registry" guard - `AppType._missing_`: both range/registry guards (same message as the first bullet) Each message was verified byte-identical old vs new (same repr()/str() semantics for %r/!r and %s/{}), and exercised through its real call path (`AppType.get('http', proto=tcp)`, `AppType.get(80, proto=undefined)`, `extend_enum(AppType, ...)`, out-of-range `AppType(...)`) with matching live exception text before and after. The edit is in the `BASE` template of `pcapkit/vendor/reg/apptype/apptype.py` (braces doubled, as an f-string template emitting f-strings) and applied by hand to the const file rather than by a crawl, to avoid rewriting the IANA-derived member rows. Expanding `BASE` with dummy TABLE/MISS and diffing against the const file shows exactly 2 non-equal regions -- the docstring's list-table and the range-row tail of `_missing_`, both TABLE/MISS-dependent -- and 0 elsewhere. Left unconverted, and said why in the PR: the three %-formatted dunders (`__new__`, `__repr__`, `__str__`) the issue also mentions -- out of the issue's stated raise-only scope, and `__new__`'s format string sets every member's underlying `StrEnum` value across all ~12,391 real members, a much larger blast radius than an error path. The module-level `consider-using-f-string` pylint disable stays: those dunders and the `extend_enum(..., '%d' % value, ...)` calls still use `%`. `tests/const/test_const_enum_builtin_parity.py::test_every_bespoke_template_ carries_the_guard` (GitHub issue #647) pinned all four bespoke vendor templates' guards against one shared `%`-style literal, so converting `reg.apptype.apptype`'s copy broke it in CI (7 legs). `BESPOKE_TEMPLATES` is now a dict keyed to each template's own guard text -- the other three still raise with `%` (#798 tracks sweeping them, the `%`-formatted dunders, and dropping the disable) -- rather than an `or` of both forms, which would let a genuinely deleted guard pass. Proved by deleting the guard from `pcapkit.vendor.tcp.flags` locally: the test failed with the expected subTest and message, then the file was restored and the tree verified clean. Build/test: `tests/const/test_const_apptype_split_unit.py` (19), `tests/vendor/test_vendor_reg_apptype_generator_unit.py` (6), and `tests/const/test_const_enum_builtin_parity.py` (20, `requests` importable so neither guard test skips) all pass under both pytest and plain unittest. mypy clean before and after. pylint: 0 new findings in either touched apptype file; R0801 unchanged at 575 tree-wide. Closes #792
`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.
`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.
`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.
…s in #817 and #821 Two bullets, both non-breaking, appended after the #800 entry in merge order. Bullet count 128 to 130 (`grep -cE '^\* \*\*'`). - #804 (PR #817) -- the three `__repr__` methods #798 left `%`-formatted are f-strings now, dropping `consider-using-f-string` from both const modules and both vendor templates; the other bespoke templates in `{const,vendor}/{ftp,http}/` still carry the disable, so #804's claim holds for this pair only, not for those directories. - #682 (PR #821) -- `TCP.__proto__` no longer binds `httpv1.HTTP` directly for ports 80/8080; both repoint to the generic HTTP proxy `_guess_version` identifies through, which only became reliable once #800/#814 landed. `udp.py` already pointed there, so that side of the PR is prose-only (its port rows and docstring), not a code change, and the entry says so. Protochain over the 23 sample captures is *not* byte-identical: 9 frames in `options-transport.pcap` go `Raw` to `HTTP/2`, all 231 HTTP/1.1 frames are unaffected, and `_guess_version`'s entry count goes 0 to 252. Not marked `**a breaking change to**`: PR #821's own labels are `bug,fix,docs,test`, no `breaking`, unlike #759/#783 and #805/#811 last round, whose crediting PRs did carry it. The entry does say what a `breaking`-blind reader would still want to know -- TCP:80/8080 traffic that is neither valid HTTP/1 nor preface-carrying now reaches `_guess_version`'s fall-through arm instead of the direct `httpv1` bind's unconditional `Raw`, which is where the 12 (of 252) fall-throughs the PR measured come from. `util/changelog_md.py` regenerated `CHANGELOG.md`, first pass, no line-spanning literal this round; `--check` exit 0. `test_changelog_md.py` 47 passed.
`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.
…s in #817 and #821 Two bullets, both non-breaking, appended after the #800 entry in merge order. Bullet count 128 to 130 (`grep -cE '^\* \*\*'`). - #804 (PR #817) -- the three `__repr__` methods #798 left `%`-formatted are f-strings now, dropping `consider-using-f-string` from both const modules and both vendor templates; the other bespoke templates in `{const,vendor}/{ftp,http}/` still carry the disable, so #804's claim holds for this pair only, not for those directories. - #682 (PR #821) -- `TCP.__proto__` no longer binds `httpv1.HTTP` directly for ports 80/8080; both repoint to the generic HTTP proxy `_guess_version` identifies through, which only became reliable once #800/#814 landed. `udp.py` already pointed there, so that side of the PR is prose-only (its port rows and docstring), not a code change, and the entry says so. Protochain over the 23 sample captures is *not* byte-identical: 9 frames in `options-transport.pcap` go `Raw` to `HTTP/2`, all 231 HTTP/1.1 frames are unaffected, and `_guess_version`'s entry count goes 0 to 252. Not marked `**a breaking change to**`: PR #821's own labels are `bug,fix,docs,test`, no `breaking`, unlike #759/#783 and #805/#811 last round, whose crediting PRs did carry it. The entry does say what a `breaking`-blind reader would still want to know -- TCP:80/8080 traffic that is neither valid HTTP/1 nor preface-carrying now reaches `_guess_version`'s fall-through arm instead of the direct `httpv1` bind's unconditional `Raw`, which is where the 12 (of 252) fall-throughs the PR measured come from. `util/changelog_md.py` regenerated `CHANGELOG.md`, first pass, no line-spanning literal this round; `--check` exit 0. `test_changelog_md.py` 47 passed.
… them, per #719 tests/ is exempt from the issue-citation rule (documentation.rst, ruled on #719): the fact cited lives in the pull request, not the issue. - PR #836 restored for the TransportProtocol-extension refusal, the |-composite decoding retirement, and the stale-comment deletion; the rulings are not on #808 at all. - PR #783 for the f-string convention; PR #847 for the mint criterion. - The de-quotation stands: wording stays as statements, no quotation marks.
…quest (#719) (#982) * docs(pcapkit,ci): cite the issue a defect belongs to, not the pull request (#719) Per the owner's ruling on #719, replace every reference to a pull-request number in pcapkit/** and .github/workflows/** comments and docstrings with the issue it closed, or a description where no issue covers it. - 92 real PR citations in pcapkit/ (93 was the estimate; the gap is RFC packet-diagram and hex-format-spec false positives, plus one cross-repo issue citation that only coincidentally matched a PyPCAPKit PR number). - 13 PR citations in .github/workflows/, matching the estimate exactly. - Several citations named two or three numbers for one claim where a PR closed several issues, or several PRs closed the same issue; deduplicated rather than left reading "#425 and #425". Four review rounds caught the same category error recurring: several sites had relocated a verbatim quote or a specific finding into the issue number rather than describing where the ruling was actually given, so the quote no longer existed where the sentence pointed. Fixed each by naming the issue while locating the ruling honestly -- "a ruling given in review of the work for #N" -- the same shape already used on this repo's conventions docs. Two sites needed the inverse correction instead: the #923 quote in enum.py/exceptions.py genuinely is recorded on #923's own thread, just attributed there to the pull request that implemented it, so those read "a ruling recorded on GitHub issue #923" rather than pointing elsewhere. Also fixed a lost conjunction and an ordinal/number mismatch in corekit/enum.py, a self-contradicting below/above pointer repeated across three internet/ files, and a number collision in http.py where one issue ended up naming both a defect and the change that closed it. Final sweep: grepped the whole tree for the word "verbatim" -- the marker that makes a quote-attribution claim falsifiable -- across all 30 files under pcapkit/ that carry it, and checked every quote this way names against the actual issue thread. Caught two more of the same defect: vendor/__main__.py's #872 citation (the quote is in the implementing pull request's review, not #872 itself) and four sites across mh.py attributing to #935 a ruling that only exists in the review of the pull request that implemented it -- #935's own thread holds just the superseded widen-not-delete proposal. Both fixed the same way. Every other quote-bearing claim the sweep found -- #911, #937, three distinct #877 quotes, both #842 quotes, and the #860/#808/#806/#886/#917 rewrites from earlier in this pass -- resolves to the thread it names. Verified: targeted pytest across every touched module passes, including the test that pins the vendor/const apptype.py get() region as byte-identical, reconfirmed after each amendment. Both edited workflow YAML files parse before and after with unchanged key counts. * docs(corekit): cite #719, not #937, for the AbsentType ruling AbsentType's docstring attributed the owner's "document it as private type/class... not for public use is enough" quote to #937. #937 itself quotes that ruling verbatim under "The owner's ruling, verbatim (from #719)" -- it re-attributes rather than originates it. Per the house rule to cite the issue a ruling was settled on (docs/source/contributing/conventions/documentation.rst:196-200), point the attribution at #719 and re-wrap the paragraph to the file's existing ~78-column width. The neighbouring, unrelated #937 citation describing what #937 did to sentinel naming is untouched. tests/corekit/ passes (400 passed, 16 skipped) against this worktree's own pcapkit (confirmed via pcapkit.__file__); pylint on the file is 9.77/10, unchanged by this edit -- the one finding is a pre-existing, unrelated too-few-public-methods warning on NoValueType. * docs(tests): re-point six ruling citations at their actual threads, per #719 Swept tests/ for owner-ruling quotes attributed to the wrong GitHub thread, the same defect class #719 fixed under pcapkit/. Confirmed each by grepping the quote's distinctive text against the cited thread's body/comments; a hit elsewhere is a re-quote or a different thread's own words, not the source. - test_sentinel_exports_unit.py: the already-reported #937->#719 fix for AbsentType's privacy ruling. - test_enum_lookup_reparent_930_unit.py (4 sites) and test_mh_unit.py: "I prefer (2) directly" and the question that drew it are in pull request #940's thread, not issue #935 -- #935 only carries the first ruling ("I lean on 1"). - test_vendor_snapshot_restore_unit.py: the contextlib/atomic-write ruling is in pull request #873's thread; issue #872 has zero comments. - test_vendor_reg_apptype_generator_unit.py (2 sites): the "undefined direct uses 0" ruling is in pull request #874's thread, not issue #860 or #770. - test_const_enum_no_mint.py (2 sites): the mint/unmint criterion was settled on pull request #847 and confirmed on #775 -- the reverse of what the text said, per #861's own description of the same ruling; and "Q1 - bare it is." is pull request #838's thread, not #775's. One occurrence left unresolved rather than guessed at: the "Preserve each branch's existing name argument..." quote (4 sites in test_const_enum_no_mint.py, attributed to "#775's final round") does not appear verbatim in #775, #847, or #878 (the implementing PR) by body, comments, review comments, or commit message -- only a paraphrase in #878's own PR description/commit message, which is the author's prose rather than a quoted ruling. Flagged for the owner rather than fixed. tests/corekit/, tests/vendor/, tests/const/ pass (400/16, 118, 299 respectively, pcapkit.__file__ confirmed inside this worktree); tests/protocols/internet/test_mh_unit.py passes standalone (52/0) -- the full directory has 5 unrelated pre-existing failures from ungenerated examples/captures/ fixtures, untouched by this change. Refs #719 * docs(tests): paraphrase four fabricated or altered owner quotations (#719) Per #719's citation ruling (de-quote, never reproduce a verbatim quote that may have come from outside GitHub): - test_const_enum_no_mint.py (4 sites): a quotation attributed to "the owner's ruling, verbatim" never appears in #775, #847, #861 or #878 (or anywhere in the repo's comment corpus). Replaced with a paraphrase attributed to PR #878's own body, which carries the real design note in different words. - test_sentinel_exports_unit.py / test_const_registry_protocol.py: a quote attributed to #911 silently dropped half of what the owner wrote on #719 and swapped `__all__` for "users". Replaced with a paraphrase naming #719 as where it was settled and #911 as the issue that carried it out. - test_const_enum_no_mint.py / test_const_enum_builtin_parity.py (4 sites): a "verbatim" quote of #860 silently corrected the owner's typo ("entires" -> "entries"). Paraphrased, which drops the question of reproducing or flagging the typo. - test_enum_lookup_reparent_930_unit.py: "the owner's final ruling there" had #935 as its nearest antecedent instead of #940; named #940 explicitly and paraphrased the adjacent quote. Verified: ast.parse and reST markup pairing clean on every touched file; tests/const (299 tests) and the targeted pytest sweep of all touched files (261 passed, 2013 subtests) are green. tests/corekit's full discover run shows 5 pre-existing failures in test_sentinel_exports_unit.py, confirmed identical on the unedited originals -- a cross-file test-order dependency unrelated to this change. * test(vendor,corekit): fix a surviving fabricated ruling and a wrong citation (#719) - tests/vendor/test_ipx_socket_unit.py: the "owner's ruling" attribution for keeping the hex-suffixed Xerox name survived in this file after the prior commit removed the same false attribution from four sites in test_const_enum_no_mint.py. Reworded to credit PR #878's own design note, matching the wording already used at the repaired sites. - tests/corekit/test_sentinel_exports_unit.py: the docstring cited the #719 export ruling ("only export objects, not types") as grounds for keeping ABSENT out of __all__, but ABSENT is an object, so that ruling argues for including it, not excluding it. Re-grounded the sentence on the privacy ruling already quoted ~15 lines below instead, without re-quoting it. Both changes are prose-only: tokenizing each file before and after with comments and docstrings stripped produces identical token sequences. tests/vendor passes 118/118 except one pre-existing, test-order-dependent flake in test_vendor_snapshot_restore_unit.py (reproduces identically on the pre-edit tree); tests/project/test_conventions_doc_claims.py passes 38/38. * test(corekit,const): narrow the blanket paraphrase, restoring quotations that cite correctly (#719) The last two commits paraphrased every disputed owner quotation away. That was right for one case and wrong for two: a quotation that exists nowhere has to be paraphrased, but a quotation that is real and was only cited to the wrong thread lost its audit trail for nothing, since the defect was the pointer, not the words. Per the owner's ruling, narrow the fix to match. Restored as quotations, correctly cited: - tests/corekit/test_sentinel_exports_unit.py (~L4-7) and tests/const/test_const_registry_protocol.py (~L1363): the sentinel export rule, split back into its two real sources instead of one spliced sentence -- #719's "we should ONLY export the objects ... and leave the types ... out", and #911's own "we only expose the final objects to users", with #911 noted as both executor and source. - tests/const/test_const_enum_no_mint.py (~L88, ~L1876, ~L2363) and tests/const/test_const_enum_builtin_parity.py (~L655): the #860 minting ruling, including its load-bearing first sentence ("I think we should not mint on get still actually") and the owner's own "entires" typo, marked [sic] rather than silently corrected. Left alone: the four #878 fabricated-quote sites in test_const_enum_no_mint.py, which cite no real thread and stay paraphrased, and the ABSENT privacy sentence, which is a correct paraphrase of a different ruling. Verified: ast.parse and reST markup clean on all four files; code token sequences (docstrings/comments stripped) identical before and after; each restored quotation substring-matches its source comment after whitespace/markup normalisation. tests/const: 299 OK. tests/ project/test_conventions_doc_claims: 38 OK, 1 skipped. * test(corekit,const): convert restored quotations to statements with context, per #719 The previous commit restored eight verbatim quotations to fix a narrowing that had dropped their context. The owner has since ruled that neither form is right: a narrowed paraphrase without context does not help a reader who was not in the thread, but a verbatim quotation makes the docstring read as a discussion rather than documentation. - Sentinel export rule (corekit/test_sentinel_exports_unit.py, const/test_const_registry_protocol.py): state that a module's `__all__` lists a sentinel's object but deliberately leaves its type out, and why (the type is not part of the public surface), citing #719 as where it was settled and #911 as where the implementing work belongs. - #860 minting rule, four sites (const/test_const_enum_no_mint.py x3, const/test_const_enum_builtin_parity.py): state that `get()` must not mint and only `register()` creates a new entry, and why (only IANA-registered values are legitimate and `get()` lacks the information to construct one), citing #860. Each site is fitted to its own surrounding prose rather than one paragraph pasted four times. Drops the `[sic]` each quotation carried, since there is nothing left to reproduce. - Fixed two sentences left orphaned by the quotations' removal: an antecedent ("the three") that depended on the deleted quote's wording, and a sentence whose "get() as well as _missing_" had the emphasis backwards relative to the rule's own subject. The four PR #878 paraphrase sites in test_const_enum_no_mint.py were already in this third form and are unchanged. Verified: ast.parse on all four files; tokenize with comments and docstrings stripped shows an identical token sequence before/after (prose-only); tests/const (299) and tests/project/test_conventions_doc_claims.py (38, 1 skip) pass; tests/corekit (400, 5 failures, 16 skipped) matches the documented pre-existing sentinel-identity failures. * test(corekit,const): state the remaining owner rulings in our own words, per #719 The four files still carried owner-attributed quotations beside the eight converted earlier, so each read half as documentation and half as a thread. - Replace each quoted ruling with a statement of the rule, the reason a reader needs, and the issue where it was given (#842, #864, #775, #860, #911, #719, #647, #808, #759, #857). - Rename the dangling "privacy ruling quoted below" reference to point at the statement that replaced the quotation. - Leave RFC text, code literals and ordinary prose untouched. Prose only: tokens with comments and docstrings stripped are identical before and after; tests/const 299 OK, tests/corekit unchanged (5 known). * test(const): restore ruling citations to the pull requests that carry them, per #719 tests/ is exempt from the issue-citation rule (documentation.rst, ruled on #719): the fact cited lives in the pull request, not the issue. - PR #836 restored for the TransportProtocol-extension refusal, the |-composite decoding retirement, and the stale-comment deletion; the rulings are not on #808 at all. - PR #783 for the f-string convention; PR #847 for the mint criterion. - The de-quotation stands: wording stays as statements, no quotation marks.
1 participant
Please follow the guide below
You will be asked some questions, please read them carefully and answer honestly
Put an
xinto all the boxes [ ] relevant to your pull request (like that [x])Use Preview tab to see how your pull request will actually look like
Searched for similar pull requests
Followed the coding style (
make pylint,make mypy,make isort)make testpasses, and a test case covers the changeAdded a changelog entry under
docs/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?
Tick the commit type your subject line carries.
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 #759.
_dispatchresolved a compositeprotoby its lowest set bit, so any composite containingtcpdispatched into the TCP registry. It now raisesProtocolErrorfrompcapkit.utilities.exceptions— aValueErrorsubclass, soget/get_all's documentedRaises:still holds, and loud rather than quiet because nothing catches it (unlike_missing_'s guard, whichConstEnumBuiltinParityTests.test_the_exception_is_not_an_in_library_onerequires to stay a non-BaseError).Blast radius, re-derived on
fe80b8525. Sweeping all 10,625 multi-transport members throughget(m.port, proto=m.proto): 0 exceptions, 0 mints, 46 answers naming a service other than the member's own, at 20 distinct ports. Those 46 are three different things, and only the last is a wrong answer: 44 differ only in that the member is not its port's canonical, which a single-bit lookup does too and whichgetdocuments; 23 — the UDP-declared half — came back as a member of the TCP registry, sotype()and.protowere wrong whatever the service string said; and 2 named a service UDP does not answer for the port at all. Those two are the whole blast radius: port 888,accessbuilder→ TCP'scddbp, and port 999,puprouter→ TCP'sgarconagainst UDP'sapplix. #759's body found the first and generalised from it. After the fix: 10,625 refusals, 0 answers.Population, since four figures here are easy to confuse. 12,391 declared members (TCP 6147, UDP 6143, SCTP 91, DCCP 10); of those 10,625 carry a multi-bit
.protoand 1,766 a single-bit one; they sit on 12,341 distinct(registry, port)pairs (TCP 6121, UDP 6119, SCTP 91, DCCP 10), the other 50 sharing a port with another service as IANA's three on TCP/80 do. The invariance digest covers those 12,341 rows — one per(registry, port), asregistry|port|resolved service|int(resolved proto), each looked up by passing the registry's own single transport bit whatever the member's.protoholds:3a457683c7b00609b06526aa02e3c361a910fa4a0e22d25ca16b4ab01acc053a, identical either side of the fix and identical again withprotopassed as a name. So it is the argument that is single-bit, not the member set — an earlier revision of this description labelled it "12,341 single-bit ports", which is a set that does not exist. Restricted to the 1,766 single-bit-.protomembers it is 1,763 rows (three share a port), digest308715ae6642a91be547d73e1c07f84bed7f27b7158951e831201c936807814b, also identical either side. All four counts are asserted in the test.Regeneration. The change is in the crawler's
BASEtemplate; the const file is regenerated from it against IANA's live CSV (sha25634328ed0…, re-fetched and unchanged), which reproduces all five committed files byte-identically before the change, so the diff is only the guard and its comment. Members unchanged. Siblings byte-identical, md5 before == after:be2b6a48tcp.py,6e169b8audp.py,39a7749dsctp.py,556f8403dccp.py.Tests.
tests/const/test_const_apptype_split_unit.py15 → 19 tests, 808 → 892 subtests, agreeing underpython -m unittest(19 OK). The two refusal tests fail against the pre-fix const file under both runners; the two invariance tests (single-bit resolution, and an AST pin on all ten library call sites passing one transport protocol) pass either side by design. Coverage of the changed file 51.752% → 51.843%: +4 statements, +2 branch arcs, 0 new misses.pylintreports the same 14 findings asmainfor these two files,mypyandisortclean.make testnot run in full — targeted selections only (tests/const,tests/vendor,tests/project= 251 tests / 2382 subtests, plus the transport/schema/corekit/foundation callers = 111 / 188).Label-worthy: this regenerates a const table, and it is a public-contract change —
AppType.get(port, proto=<composite>)previously returned a member and now raises. No in-library caller passes a composite;register_apptypereads a member's compositeprotobut tests it withinand never reaches the lookup.