Repository navigation
fix(registry): add register_protocol's identity guard to sibling registrars - #726
Conversation
JarryShaw
added
bug
|
❌ NEEDS CHANGES @ |
|
❌ NEEDS CHANGES @ Independent cross-review, Opus (author: Sonnet). Verified on Blocker. Post-fix all three are silent, so the guard is live rather than symmetry-only. Not a contrived shape either: #721 just put that list form into production ( Also.
Reach is narrower than the body implies. Seeded registries hold
Runs (3.14.7, #726 vs #728: disjoint library files, and the shared ❌ NEEDS CHANGES @ |
d12c3b4 to
6634e22
Compare
JarryShaw
added
the
review: pending
|
❌ NEEDS CHANGES @ |
|
❌ NEEDS CHANGES @ Independent cross-review, Opus (author: Sonnet). Supersedes my Blocker 1 —
Correcting those two sentences is enough to clear this; the guard itself is fair follow-up, since #728 rewrites that file. Blocker 2 — the body's coverage pair does not reproduce. Under the body's own scope (nine modified test files as the pytest selection, eight modified library files as the coverage scope), both sides measured:
Also measured. Merged into current ❌ NEEDS CHANGES @ |
JarryShaw
added
review: needs-changes
6634e22 to
22b5739
Compare
JarryShaw
added
review: pending
|
Addressed both remaining items from the last review:
Amended onto the existing commit, force-pushed. New head: |
22b5739 to
63dce26
Compare
JarryShaw
changed the title
|
Closed the one holdout: Two new tests, both shown failing against the unfixed guard before the fix:
PR title/body and table updated to ten registrars. |
|
❌ NEEDS CHANGES @ |
|
❌ NEEDS CHANGES @ Independent cross-review, Opus (author: Sonnet). Supersedes my
Insertion invariant — holds, in both directions.
One real "warns less than it should". Follow-up, not a blocker. Class-valued presence-only registrars survive outside #718's "renders Merged into current ❌ NEEDS CHANGES @ |
JarryShaw
added
review: needs-changes
63dce26 to
3bbc4e6
Compare
JarryShaw
added
review: pending
|
Addressed cross-review's three prose sites (head
Grepped all 9 test files this PR touches for the same phrasing ( Re-ran the three affected test files: 104 passed, 1838 subtests, exit 0. |
|
❌ NEEDS CHANGES @ |
|
❌ NEEDS CHANGES @ Independent cross-review, Opus (author: Sonnet). Supersedes my The blocker. Fix 1 removed the two false sentences correctly, but its replacement ends on a package-wide universal that does not hold. 23 presence-only
Sweep spot-checked, not trusted. Your five terms ( CI, read directly. 22 pass / 0 fail / 0 cancelled. The three One provenance caveat on my own evidence. The "15 failures across 9 files on full revert" figure remains single-sourced from my measurement agent; I have not re-derived it personally. Its composition, so it is checkable: 6 failures in ❌ NEEDS CHANGES @ |
…strars - Ten code-keyed registrars (ProtocolBase, Internet, Link, Transport, SCTP, Frame, PCAPNG, EnumSchema's register + __init_subclass__, and pcapng.py's Option.register) warned on mere presence, so re-registering the exact same class under the same code emitted a misleading "overwriting X with X". Guard each on presence AND identity, matching register_protocol's guard from #681/#711. - Option.register needed its own fix: __init_subclass__ loops over a code list with no deduplication, so code=[b, b] reached it twice with the same class and warned about a self-overwrite. Rewrote its docstring, dropping the now-false admission that this could not happen. - Updated each site's docstring: the "fires on presence alone, deliberate" rationale (added by #695) no longer holds now the guard changed. - Repurposed test_sibling_registries_still_warn_on_an_identical_re_registration (tests/foundation/registry/test_protocols.py) and fixed five other pre-existing tests that re-registered a literal same object as their "overwrite" case. - Added one same-object no-op test per site (nine total), plus a tenth covering EnumSchema.__init_subclass__'s class-declaration path, and two more for Option.register's own code=[b, b] shape (silent on the same class, still warns once on a genuine displacement). - Cross-review found three prose sites that still argued the rejected reasoning: register_protocol's own docstring (foundation/registry/ protocols.py) claiming every sibling warns on mere presence, a test_pcapng_unit.py test docstring claiming __init_subclass__ passes each code exactly once, and a one-line summary in test_enum_schema_registry_unit.py calling the guard presence-only. Fixed all three; grepped every test file this PR touches for the same phrasing, no further instances. Did not apply #711's id() disambiguation to the siblings -- see PR body. Build: targeted pytest run (9 files), 204 passed, 1930 subtests, exit 0. Option.register: test_pcapng_unit.py, 81 passed, 1753 subtests, pcapng.py at 100% line/branch coverage, exit 0. Re-verified with the two other touched test files: 104 passed, 1838 subtests, exit 0. Fixes #718.
3bbc4e6 to
0f84641
Compare
JarryShaw
added
review: pending
|
✅ GOOD TO MERGE @ |
|
✅ GOOD TO MERGE @ Independent cross-review, Opus (author: Sonnet). Supersedes my The replacement is true, and I checked the scope rather than the wording. It now reads "…apply the same identity criterion as of GitHub issue #718; before that they warned on presence alone, and none of them does now", whose subject is explicitly "the sibling Sweep re-run, not assumed. Your five terms ( CI, read directly at this head. 20 pass, 0 fail, 0 cancelled, 3 Three caveats that survive to merge, none blocking. The ✅ GOOD TO MERGE @ |
JarryShaw
added
review: good-to-go
…t missed register_engine, register_reassembly, register_traceflow, and both register_dumper copies (Extractor and TraceFlowBase) warned on mere key presence, so re-registering the exact same class or descriptor read as "overwriting X" even though nothing was displaced. #718 fixed this for ten code-keyed registrars but excluded these five because their message wording falls outside its criterion, even though they share the defect. - Replace each `if key in cls.__dict__:` guard with `incumbent = cls.__dict__ .get(key)` + `incumbent is not None and incumbent is not value`, matching the identity-guard shape #726 gives the ten sibling registrars. Message text is unchanged. - register_engine/reassembly/traceflow: all three seed dicts hold only ModuleDescriptors, never a class, so the *first* real registration (descriptor -> class) still legitimately warns; only a *repeat* registration is now silent. Plain dicts, so `.get()`/`in` are both non-inserting. - register_dumper (both copies): __output__ maps format -> (dumper, ext), so the identity check compares the incumbent dumper (index 0), not the pair -- a re-registration that only changes ext still counts as a different entry. __output__ is a `collections.defaultdict`; measured that `.get()` does not invoke the default factory the way a subscript access would (confirmed: len() unchanged on a `.get()`/`in` miss, +1 on a `d[missing]` subscript). - Add tests pinning, per registrar: same-object re-registration is silent, different-object replacement still warns with the same message, and the registry write happens on both paths. Verified all five fail against the unfixed guard. Ran tests/foundation/test_extraction.py + tests/foundation/traceflow/ test_traceflow_base.py: 19 -> 21 passed (28 subtests). coverage: extraction.py 370 -> 372 stmts (miss unchanged at 7, 96%), traceflow.py 121 -> 123 stmts (0 miss, 100%) -- all new statements fully exercised. Fixes #739.
Library modules imported a *Base class aliased to its public name, so the source read `class PCAP(Engine)` while actually inheriting `EngineBase`. Per the ruling on #514 the split is permanent and library classes inherit the base, so the base is now imported under its own name. * rewrote 28 of the 82 alias imports -- EngineBase 8 of 8, ReassemblyBase 3 of 3, DumperBase 2 of 2, TraceFlowBase 2 of 2, ProtocolBase 13 of 67 * renamed the code and annotation references that followed, including `cast()` targets, `TypeVar(bound=)` and `# type:` comments, but not the string values inside `Literal[...]` * corrected four `docs/source` index pages that named the public class while their own class diagram roots the hierarchy at the base * added `tests/test_base_class_contract.py`, pinning the contract per suite The four non-protocols families are complete. The remaining 54 sites are all `ProtocolBase` ones in paths #726 and #742 own, recorded as prefixes in `PENDING_ALIAS_PATHS` so the entry fails once its pull request lands. No behaviour change: name registry 38 keys before and after, `descendants(Public)` 0 in all five suites, 45 dispatches identical and still identical after clearing the name registry, mypy at its 112-error baseline with none introduced.
…t missed register_engine, register_reassembly, register_traceflow, and both register_dumper copies (Extractor and TraceFlowBase) warned on mere key presence, so re-registering the exact same class or descriptor read as "overwriting X" even though nothing was displaced. #718 fixed this for ten code-keyed registrars but excluded these five because their message wording falls outside its criterion, even though they share the defect. - Replace each `if key in cls.__dict__:` guard with `incumbent = cls.__dict__ .get(key)` + `incumbent is not None and incumbent is not value`, matching the identity-guard shape #726 gives the ten sibling registrars. Message text is unchanged. - register_engine/reassembly/traceflow: all three seed dicts hold only ModuleDescriptors, never a class, so the *first* real registration (descriptor -> class) still legitimately warns; only a *repeat* registration is now silent. Plain dicts, so `.get()`/`in` are both non-inserting. - register_dumper (both copies): __output__ maps format -> (dumper, ext), so the identity check compares the incumbent dumper (index 0), not the pair -- a re-registration that only changes ext is still identity-equal on the dumper and stays silent. __output__ is a `collections.defaultdict`; measured that `.get()` does not invoke the default factory the way a subscript access would (confirmed: len() unchanged on a `.get()`/`in` miss, +1 on a `d[missing]` subscript). `incumbent_entry[0]` is safe on a factory-produced miss too, since the factory itself returns a 2-tuple. - Add tests pinning, per registrar: same-object re-registration is silent, different-object replacement still warns with the same message, and the registry write happens on both paths. Verified all five fail against the unfixed guard. Ran tests/foundation/test_extraction.py + tests/foundation/traceflow/ test_traceflow_base.py: 16 -> 21 passed (28 subtests, unchanged). coverage: extraction.py 367 -> 372 stmts (miss unchanged at 7, 96%), traceflow.py 121 -> 123 stmts (0 miss, 100%) -- all new statements fully exercised. Fixes #739.
…t missed (#742) register_engine, register_reassembly, register_traceflow, and both register_dumper copies (Extractor and TraceFlowBase) warned on mere key presence, so re-registering the exact same class or descriptor read as "overwriting X" even though nothing was displaced. #718 fixed this for ten code-keyed registrars but excluded these five because their message wording falls outside its criterion, even though they share the defect. - Replace each `if key in cls.__dict__:` guard with `incumbent = cls.__dict__ .get(key)` + `incumbent is not None and incumbent is not value`, matching the identity-guard shape #726 gives the ten sibling registrars. Message text is unchanged. - register_engine/reassembly/traceflow: all three seed dicts hold only ModuleDescriptors, never a class, so the *first* real registration (descriptor -> class) still legitimately warns; only a *repeat* registration is now silent. Plain dicts, so `.get()`/`in` are both non-inserting. - register_dumper (both copies): __output__ maps format -> (dumper, ext), so the identity check compares the incumbent dumper (index 0), not the pair -- a re-registration that only changes ext is still identity-equal on the dumper and stays silent. __output__ is a `collections.defaultdict`; measured that `.get()` does not invoke the default factory the way a subscript access would (confirmed: len() unchanged on a `.get()`/`in` miss, +1 on a `d[missing]` subscript). `incumbent_entry[0]` is safe on a factory-produced miss too, since the factory itself returns a 2-tuple. - Add tests pinning, per registrar: same-object re-registration is silent, different-object replacement still warns with the same message, and the registry write happens on both paths. Verified all five fail against the unfixed guard. Ran tests/foundation/test_extraction.py + tests/foundation/traceflow/ test_traceflow_base.py: 16 -> 21 passed (28 subtests, unchanged). coverage: extraction.py 367 -> 372 stmts (miss unchanged at 7, 96%), traceflow.py 121 -> 123 stmts (0 miss, 100%) -- all new statements fully exercised. Fixes #739.
…one (#750) Library modules imported a *Base class aliased to its public name, so the source read `class PCAP(Engine)` while actually inheriting `EngineBase`. Per the ruling on #514 the split is permanent and library classes inherit the base, so the base is now imported under its own name. * rewrote 28 of the 82 alias imports -- EngineBase 8 of 8, ReassemblyBase 3 of 3, DumperBase 2 of 2, TraceFlowBase 2 of 2, ProtocolBase 13 of 67 * renamed the code and annotation references that followed, including `cast()` targets, `TypeVar(bound=)` and `# type:` comments, but not the string values inside `Literal[...]` * corrected four `docs/source` index pages that named the public class while their own class diagram roots the hierarchy at the base * added `tests/test_base_class_contract.py`, pinning the contract per suite The four non-protocols families are complete. The remaining 54 sites are all `ProtocolBase` ones in paths #726 and #742 own, recorded as prefixes in `PENDING_ALIAS_PATHS` so the entry fails once its pull request lands. No behaviour change: name registry 38 keys before and after, `descendants(Public)` 0 in all five suites, 45 dispatches identical and still identical after clearing the name registry, mypy at its 112-error baseline with none introduced.
…arning count Round 8: ran the merge-base/changed-files/cited-path intersection to completion (0c7f2b7..origin/main: 43 commits/124 files; 54 cited paths, 34 of them touched by that range) instead of trusting a tense grep. - :1540/:1542 -- "93 of the 95 sites"/"48 of the 49 record lengths" -> 94 of 95 / 49 of 49; zero old-expression sites remain on main. - :1544-1546, :1566 -- "LOCATOR_SET keeps the old expression ... is currently right" / "is unchanged in both respects" -> past tense; #679 fixed both LOCATOR_SET sites. - :1566 -- "HIP_COPIES stays at two" -> past tense; #679 fixed LOCATOR_SET's Length unit, #689 then dropped HIP_COPIES to one. - :1976-1977 -- "pcapng.txt ... wants a separate refresh" -> past tense; #685 removed it from the index instead of regenerating it. - :2327-2328 -- "55 warnings on main before this change, 56 after" (-b html) -> 53/54; the raw `grep -c WARNING:` double-counts two Scapy import lines as Sphinx warnings, confirmed live on this head with both -b dummy and -b html (real 36/raw 38 with const/reg.rst excluded, same +2 gap either way). - :834 -- stale ``protocol.py:1016`` -> ``:1413`` (the actual ``self._file.read()`` call inside ``_read_fileng``). - :1969 -- the #646 entry's coverage renumbering was wrong twice over (first ``1153 to 1265``, then ``1153 to 1443``, the latter being main's ``def`` line, which always executes and can never be the single miss). Corrected to ``1248 to 1360``, the ``warn(...)`` statement's line before/after #646's own diff. Regenerated CHANGELOG.md from the edited entries. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests. Follow-up: origin/main advanced through #726/#740/#741/#742 (to 0a3abff) and then #747/#748 (to 074c53e) while this sat at good-to-go; #726 moved three more claims anchored on files it touched. - :834 -- ``protocol.py:1413`` -> ``:1411``; #726 shifted the ``self._file.read()`` call in ``_read_fileng`` by -2 lines. - :2382-83 -- traceflow.py "Line 406" -> "Line 424"; #742 inserted 18 lines above the ``#: Type[Dumper]: Dumper class.`` comment. - :2099-2104, :2187-89 -- the "seven code-keyed parser registrars" and ``Option.register`` are no longer presence-only. #726, fixing #718, gave all seven -- and ``Option.register`` itself -- the same identity guard ``register_protocol`` already had; reworded both passages to say so, confirmed against the guards' own current docstrings. Regenerated CHANGELOG.md again. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests.
…arning count Round 8: ran the merge-base/changed-files/cited-path intersection to completion (0c7f2b7..origin/main: 43 commits/124 files; 54 cited paths, 34 of them touched by that range) instead of trusting a tense grep. - :1540/:1542 -- "93 of the 95 sites"/"48 of the 49 record lengths" -> 94 of 95 / 49 of 49; zero old-expression sites remain on main. - :1544-1546, :1566 -- "LOCATOR_SET keeps the old expression ... is currently right" / "is unchanged in both respects" -> past tense; #679 fixed both LOCATOR_SET sites. - :1566 -- "HIP_COPIES stays at two" -> past tense; #679 fixed LOCATOR_SET's Length unit, #689 then dropped HIP_COPIES to one. - :1976-1977 -- "pcapng.txt ... wants a separate refresh" -> past tense; #685 removed it from the index instead of regenerating it. - :2327-2328 -- "55 warnings on main before this change, 56 after" (-b html) -> 53/54; the raw `grep -c WARNING:` double-counts two Scapy import lines as Sphinx warnings, confirmed live on this head with both -b dummy and -b html (real 36/raw 38 with const/reg.rst excluded, same +2 gap either way). - :834 -- stale ``protocol.py:1016`` -> ``:1413`` (the actual ``self._file.read()`` call inside ``_read_fileng``). - :1969 -- the #646 entry's coverage renumbering was wrong twice over (first ``1153 to 1265``, then ``1153 to 1443``, the latter being main's ``def`` line, which always executes and can never be the single miss). Corrected to ``1248 to 1360``, the ``warn(...)`` statement's line before/after #646's own diff. Regenerated CHANGELOG.md from the edited entries. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests. Follow-up: origin/main advanced through #726/#740/#741/#742 (to 0a3abff) and then #747/#748 (to 074c53e) while this sat at good-to-go; #726 moved three more claims anchored on files it touched. - :834 -- ``protocol.py:1413`` -> ``:1411``; #726 shifted the ``self._file.read()`` call in ``_read_fileng`` by -2 lines. - :2382-83 -- traceflow.py "Line 406" -> "Line 424"; #742 inserted 18 lines above the ``#: Type[Dumper]: Dumper class.`` comment. - :2099-2104, :2187-89 -- the "seven code-keyed parser registrars" and ``Option.register`` are no longer presence-only. #726, fixing #718, gave all seven -- and ``Option.register`` itself -- the same identity guard ``register_protocol`` already had; reworded both passages to say so, confirmed against the guards' own current docstrings. Regenerated CHANGELOG.md again. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests. Cross-review at 948ac49 came back NEEDS CHANGES: the round-12 edit fixed two sites of the harmonisation claim and left its twin, plus its own reasoning, asserting the opposite; and four numbers anchored on files the merges touched had drifted independently of #726. - :1877-1878, :1883-1884 (#675) -- "carries the guarded ``if code in cls.__xxx__: warn(...)``" / "every sibling warns on mere presence" -> past tense, noting #726 later gave all seven the identity guard this entry's own comparison assumes they lack. - :2100-2109 -- dropped the retained "yields two keys and never reaches one key twice" (false: ``Internet.register(TransType.TCP, TCP)`` warns once, incumbent.klass is TCP) and "leaves a different-class test undecidable" (contradicted by :2404-2406's own ``incumbent is not protocol`` definition); replaced with the actual false positive the guard has -- pre-seeded ``ModuleDescriptor`` incumbents never compare equal to the resolved class. - :2192 -- reflowed the ``Option.register`` paragraph (orphan lines fixed alongside). - :2387-2388 -- the ``Type[Dumper]`` quote now matches what is actually at line 424 (post-#709-fix), rather than the pre-fix bare form. - :945 -- ``README.md`` (103) -> (102). - :1136 -- "75 of the 117 modules" -> "77 ... after #647 below adds the same ending to three more" (drifted via #647, independent of the four merges). - :2119 -- dropped the irreproducible pylint "364 messages" figure; kept mypy's 112, which does reproduce. - :2119 -- "326 registry writes" -> 327 (``R1CounterParameter``'s second code, from #690). Also fixed six false claims in the PR body (separate from the .rst): hunk/line counts, six-commits -> 46, the 5-row table's implied total, "not trimmed", main's red/green state, and the now-unreachable cherry-pick target. Regenerated CHANGELOG.md again. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests.
…arning count Round 8: ran the merge-base/changed-files/cited-path intersection to completion (0c7f2b7..origin/main: 43 commits/124 files; 54 cited paths, 34 of them touched by that range) instead of trusting a tense grep. - :1540/:1542 -- "93 of the 95 sites"/"48 of the 49 record lengths" -> 94 of 95 / 49 of 49; zero old-expression sites remain on main. - :1544-1546, :1566 -- "LOCATOR_SET keeps the old expression ... is currently right" / "is unchanged in both respects" -> past tense; #679 fixed both LOCATOR_SET sites. - :1566 -- "HIP_COPIES stays at two" -> past tense; #679 fixed LOCATOR_SET's Length unit, #689 then dropped HIP_COPIES to one. - :1976-1977 -- "pcapng.txt ... wants a separate refresh" -> past tense; #685 removed it from the index instead of regenerating it. - :2327-2328 -- "55 warnings on main before this change, 56 after" (-b html) -> 53/54; the raw `grep -c WARNING:` double-counts two Scapy import lines as Sphinx warnings, confirmed live on this head with both -b dummy and -b html (real 36/raw 38 with const/reg.rst excluded, same +2 gap either way). - :834 -- stale ``protocol.py:1016`` -> ``:1413`` (the actual ``self._file.read()`` call inside ``_read_fileng``). - :1969 -- the #646 entry's coverage renumbering was wrong twice over (first ``1153 to 1265``, then ``1153 to 1443``, the latter being main's ``def`` line, which always executes and can never be the single miss). Corrected to ``1248 to 1360``, the ``warn(...)`` statement's line before/after #646's own diff. Regenerated CHANGELOG.md from the edited entries. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests. Follow-up: origin/main advanced through #726/#740/#741/#742 (to 0a3abff) and then #747/#748 (to 074c53e) while this sat at good-to-go; #726 moved three more claims anchored on files it touched. - :834 -- ``protocol.py:1413`` -> ``:1411``; #726 shifted the ``self._file.read()`` call in ``_read_fileng`` by -2 lines. - :2382-83 -- traceflow.py "Line 406" -> "Line 424"; #742 inserted 18 lines above the ``#: Type[Dumper]: Dumper class.`` comment. - :2099-2104, :2187-89 -- the "seven code-keyed parser registrars" and ``Option.register`` are no longer presence-only. #726, fixing #718, gave all seven -- and ``Option.register`` itself -- the same identity guard ``register_protocol`` already had; reworded both passages to say so, confirmed against the guards' own current docstrings. Regenerated CHANGELOG.md again. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests. Cross-review at 948ac49 came back NEEDS CHANGES: the round-12 edit fixed two sites of the harmonisation claim and left its twin, plus its own reasoning, asserting the opposite; and four numbers anchored on files the merges touched had drifted independently of #726. - :1877-1878, :1883-1884 (#675) -- "carries the guarded ``if code in cls.__xxx__: warn(...)``" / "every sibling warns on mere presence" -> past tense, noting #726 later gave all seven the identity guard this entry's own comparison assumes they lack. - :2100-2109 -- dropped the retained "yields two keys and never reaches one key twice" (false: ``Internet.register(TransType.TCP, TCP)`` warns once, incumbent.klass is TCP) and "leaves a different-class test undecidable" (contradicted by :2404-2406's own ``incumbent is not protocol`` definition); replaced with the actual false positive the guard has -- pre-seeded ``ModuleDescriptor`` incumbents never compare equal to the resolved class. - :2192 -- reflowed the ``Option.register`` paragraph (orphan lines fixed alongside). - :2387-2388 -- the ``Type[Dumper]`` quote now matches what is actually at line 424 (post-#709-fix), rather than the pre-fix bare form. - :945 -- ``README.md`` (103) -> (102). - :1136 -- "75 of the 117 modules" -> "77 ... after #647 below adds the same ending to three more" (drifted via #647, independent of the four merges). - :2119 -- dropped the irreproducible pylint "364 messages" figure; kept mypy's 112, which does reproduce. - :2119 -- "326 registry writes" -> 327 (``R1CounterParameter``'s second code, from #690). Also fixed six false claims in the PR body (separate from the .rst): hunk/line counts, six-commits -> 46, the 5-row table's implied total, "not trimmed", main's red/green state, and the now-unreachable cherry-pick target. Regenerated CHANGELOG.md again. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests. Cross-review at 1749cc0 came back NEEDS CHANGES: round 13 fixed five of the nine sites and introduced four new false claims doing it, including two inside the flagship rewrite -- swapping one inaccuracy for another is this document's recurring failure mode. - :1136-37 -- "75 ... 77 now, after #647 ... adds ... three more" was internally inconsistent (75+3=78, not 77). Traced #647's own diff (fc32d1b): it adds ``_missing_`` to three IntFlag classes across only two *new* files -- ``tcp/flags.py`` and ``ftp/command.py`` -- since the third, ``TransportProtocol``, shares ``reg/apptype.py`` with the already-counted ``AppType``. Module delta is +2, matching 75+2=77; reworded to say so. - :1879-88 -- dropped "the comparison below assumes they still lack" it, which was false about text 8 lines below in the same diff (already past-tensed). Also reflowed three orphan lines this introduced (`passes, whereas`, `it twice with nothing`, `none of the`). - :2107-19 -- "These tables also ship pre-seeded" over-generalised: verified live (``ProtocolBase.__proto__`` is 0 entries, ``Transport.__proto__ is ProtocolBase.__proto__`` -- True) that 2 of 7 have nothing pre-seeded. Scoped to the five that do (Link 7, Internet 16, Frame 3, PCAPNG 3, SCTP 2). Also fixed "the guard resolves only the incoming class", which contradicts the guard's own docstring ("the comparison itself resolves nothing") -- resolution is the earlier ``isinstance(protocol, ModuleDescriptor)`` step, three lines above the guard, not something the guard does. - :2129-30 -- dropped the invented "327th" ordinal (327 total stays; traced-write instrumentation via ``sys`` hooks found the seeding is literal dict construction, not ``.register()`` calls, so I could not reproduce an ordinal with confidence -- said "one of them" instead of guessing). - :7-8 -- "between #326 and #509" now says the programme continued past it (verified: 193 distinct #nnn refs, max #726, 103 above 509). - PR body -- "7 hunks, 1168+/11-" was the previous head's figure, not this one's; replaced with the actual command (``git diff --shortstat da697fa -- docs/source/changelog/1.5.0.rst``) and today's figure (9 hunks, 1152+/16-), since a hardcoded count here has now gone stale twice. Left alone per this round's scope: :1969/:1974 (before/after claim, not falsified by #726's later +1), mypy "112" (correct, re-ran with the project's own flags), ":2122" 13-to-14 (correct at its delta scope), and the other 121 cited paths (unaffected by main's one new commit, #745, confirmed test-only). Regenerated CHANGELOG.md again. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests.
…arning count Round 8: ran the merge-base/changed-files/cited-path intersection to completion (0c7f2b7..origin/main: 43 commits/124 files; 54 cited paths, 34 of them touched by that range) instead of trusting a tense grep. - :1540/:1542 -- "93 of the 95 sites"/"48 of the 49 record lengths" -> 94 of 95 / 49 of 49; zero old-expression sites remain on main. - :1544-1546, :1566 -- "LOCATOR_SET keeps the old expression ... is currently right" / "is unchanged in both respects" -> past tense; #679 fixed both LOCATOR_SET sites. - :1566 -- "HIP_COPIES stays at two" -> past tense; #679 fixed LOCATOR_SET's Length unit, #689 then dropped HIP_COPIES to one. - :1976-1977 -- "pcapng.txt ... wants a separate refresh" -> past tense; #685 removed it from the index instead of regenerating it. - :2327-2328 -- "55 warnings on main before this change, 56 after" (-b html) -> 53/54; the raw `grep -c WARNING:` double-counts two Scapy import lines as Sphinx warnings, confirmed live on this head with both -b dummy and -b html (real 36/raw 38 with const/reg.rst excluded, same +2 gap either way). - :834 -- stale ``protocol.py:1016`` -> ``:1413`` (the actual ``self._file.read()`` call inside ``_read_fileng``). - :1969 -- the #646 entry's coverage renumbering was wrong twice over (first ``1153 to 1265``, then ``1153 to 1443``, the latter being main's ``def`` line, which always executes and can never be the single miss). Corrected to ``1248 to 1360``, the ``warn(...)`` statement's line before/after #646's own diff. Regenerated CHANGELOG.md from the edited entries. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests. Follow-up: origin/main advanced through #726/#740/#741/#742 (to 0a3abff) and then #747/#748 (to 074c53e) while this sat at good-to-go; #726 moved three more claims anchored on files it touched. - :834 -- ``protocol.py:1413`` -> ``:1411``; #726 shifted the ``self._file.read()`` call in ``_read_fileng`` by -2 lines. - :2382-83 -- traceflow.py "Line 406" -> "Line 424"; #742 inserted 18 lines above the ``#: Type[Dumper]: Dumper class.`` comment. - :2099-2104, :2187-89 -- the "seven code-keyed parser registrars" and ``Option.register`` are no longer presence-only. #726, fixing #718, gave all seven -- and ``Option.register`` itself -- the same identity guard ``register_protocol`` already had; reworded both passages to say so, confirmed against the guards' own current docstrings. Regenerated CHANGELOG.md again. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests. Cross-review at 948ac49 came back NEEDS CHANGES: the round-12 edit fixed two sites of the harmonisation claim and left its twin, plus its own reasoning, asserting the opposite; and four numbers anchored on files the merges touched had drifted independently of #726. - :1877-1878, :1883-1884 (#675) -- "carries the guarded ``if code in cls.__xxx__: warn(...)``" / "every sibling warns on mere presence" -> past tense, noting #726 later gave all seven the identity guard this entry's own comparison assumes they lack. - :2100-2109 -- dropped the retained "yields two keys and never reaches one key twice" (false: ``Internet.register(TransType.TCP, TCP)`` warns once, incumbent.klass is TCP) and "leaves a different-class test undecidable" (contradicted by :2404-2406's own ``incumbent is not protocol`` definition); replaced with the actual false positive the guard has -- pre-seeded ``ModuleDescriptor`` incumbents never compare equal to the resolved class. - :2192 -- reflowed the ``Option.register`` paragraph (orphan lines fixed alongside). - :2387-2388 -- the ``Type[Dumper]`` quote now matches what is actually at line 424 (post-#709-fix), rather than the pre-fix bare form. - :945 -- ``README.md`` (103) -> (102). - :1136 -- "75 of the 117 modules" -> "77 ... after #647 below adds the same ending to three more" (drifted via #647, independent of the four merges). - :2119 -- dropped the irreproducible pylint "364 messages" figure; kept mypy's 112, which does reproduce. - :2119 -- "326 registry writes" -> 327 (``R1CounterParameter``'s second code, from #690). Also fixed six false claims in the PR body (separate from the .rst): hunk/line counts, six-commits -> 46, the 5-row table's implied total, "not trimmed", main's red/green state, and the now-unreachable cherry-pick target. Regenerated CHANGELOG.md again. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests. Cross-review at 1749cc0 came back NEEDS CHANGES: round 13 fixed five of the nine sites and introduced four new false claims doing it, including two inside the flagship rewrite -- swapping one inaccuracy for another is this document's recurring failure mode. - :1136-37 -- "75 ... 77 now, after #647 ... adds ... three more" was internally inconsistent (75+3=78, not 77). Traced #647's own diff (fc32d1b): it adds ``_missing_`` to three IntFlag classes across only two *new* files -- ``tcp/flags.py`` and ``ftp/command.py`` -- since the third, ``TransportProtocol``, shares ``reg/apptype.py`` with the already-counted ``AppType``. Module delta is +2, matching 75+2=77; reworded to say so. - :1879-88 -- dropped "the comparison below assumes they still lack" it, which was false about text 8 lines below in the same diff (already past-tensed). Also reflowed three orphan lines this introduced (`passes, whereas`, `it twice with nothing`, `none of the`). - :2107-19 -- "These tables also ship pre-seeded" over-generalised: verified live (``ProtocolBase.__proto__`` is 0 entries, ``Transport.__proto__ is ProtocolBase.__proto__`` -- True) that 2 of 7 have nothing pre-seeded. Scoped to the five that do (Link 7, Internet 16, Frame 3, PCAPNG 3, SCTP 2). Also fixed "the guard resolves only the incoming class", which contradicts the guard's own docstring ("the comparison itself resolves nothing") -- resolution is the earlier ``isinstance(protocol, ModuleDescriptor)`` step, three lines above the guard, not something the guard does. - :2129-30 -- dropped the invented "327th" ordinal (327 total stays; traced-write instrumentation via ``sys`` hooks found the seeding is literal dict construction, not ``.register()`` calls, so I could not reproduce an ordinal with confidence -- said "one of them" instead of guessing). - :7-8 -- "between #326 and #509" now says the programme continued past it (verified: 193 distinct #nnn refs, max #726, 103 above 509). - PR body -- "7 hunks, 1168+/11-" was the previous head's figure, not this one's; replaced with the actual command (``git diff --shortstat da697fa -- docs/source/changelog/1.5.0.rst``) and today's figure (9 hunks, 1152+/16-), since a hardcoded count here has now gone stale twice. Left alone per this round's scope: :1969/:1974 (before/after claim, not falsified by #726's later +1), mypy "112" (correct, re-ran with the project's own flags), ":2122" 13-to-14 (correct at its delta scope), and the other 121 cited paths (unaffected by main's one new commit, #745, confirmed test-only). Regenerated CHANGELOG.md again. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests. Cross-review at b2ac58b came back NEEDS CHANGES: round 15 fixed four sites clean but swapped in two new inaccuracies, and left one round-fourteen defect (body :26) unfixed. - :7 -- "past #726" was wrong direction: #726 is the max ref in the document (193 distinct, min #251, max #726), not one exceeded -> "reaching #726". - :2110-19 -- "ProtocolBase and Transport share one dict, starting and staying empty until a subclass registers" was false three ways, verified live against origin/main (pcapkit.__file__ asserted): Transport.register() itself raises UnsupportedCall (abstract); TCP and UDP keep their own separate __proto__ (4 and 3 entries), not the shared one, so registering on them leaves the shared dict at 0; only a direct ProtocolBase.register() call fills it. Narrowing to "five of these seven" also hid that TCP/UDP are pre-seeded too, which is exactly where the false positive bites in the transport family -- restored that. - body :26 -- "26 entry commits" -> 27 (commits whose subject starts "docs(changelog): the 1.5.0 entry/entries for", verified by grep), 28 bullets added and 0 removed (verified via the .rst diff against da697fa; one commit, 6a956c4, adds two bullets for #648/#649). - body :41 -- dropped the hardcoded "9 hunks, 1152+/16-" figure entirely (it had already drifted to 1154+ by the time of this commit) and named the second command needed for the hunk count, since --shortstat cannot print one. On the ordinal question raised last round: dropping it was still right (the asserted "327th" was wrong), but "no ordinal is derivable" does not hold either -- the writes are at pcapkit/protocols/schema/schema.py, not the 8 dict-literal registrar sites my instrumentation covered, and they are traceable. Left the text as "one of them being R1CounterParameter's second code" (no ordinal asserted, no false derivability claim either) rather than reopen a site outside this round's scope. Regenerated CHANGELOG.md again. changelog_md.py --check: exit 0. pytest tests/project/test_changelog_md.py -q: 47 passed, 37 subtests.
…st 54 sites (#752) Completes #514 part (c). #750 renamed 28 of 82 `ProtocolBase as Protocol` alias imports and deferred the remaining 54 -- all `ProtocolBase` -- to paths #726 and #742 owned. Both have merged, so the deferral is over. * renamed the alias import at all 54 sites (51 under pcapkit/protocols/, 3 under pcapkit/foundation/) and every in-file reference that used the local alias: class headers, annotations, cast(), isinstance/issubclass checks, # type: comments, and the bracketed part of a handful of Sphinx #: doc comments -- 191 lines changed, no statements added * merged two now-unaliased same-module imports per isort in 4 files (application.py, internet.py, link.py, transport.py), removing 4 statements * left descriptive prose, protocol-name string literals, error-message text, and fully-qualified :class:/:meth:/:rtype: cross-references to the real public Protocol class untouched, matching #750's own precedent * emptied tests/test_base_class_contract.py's PENDING_ALIAS_PATHS, its documented end state, and updated the stale docstring narrative No behaviour change: __mro__/__bases__/__module__ identical across 18 classes spanning every family, __proto__ registry 38 keys before and after, descendants(Protocol) 0 in both. mypy stays at the 112-error baseline.
JarryShaw
removed
the
review: good-to-go
…k included `main` moved from 73f09ae to 4530424 while this PR sat open, and the 1.5.0 section cited none of the 25 commits in between. Ten new bullets cover thirteen of them, appended in merge order, with the file's own `**a breaking change**` lead sentence on the three that are breaking: - #754 -- AppType split into per-transport registries; the 1,004 portless and 704 transportless rows stop being members. Breaking. - #764 -- an out-of-range port in `AppType.get` is refused, not minted, so `TCP.make(srcport=99999)` raises; per-transport `_missing_` spans. Breaking. - #778 -- `@final` enforced at runtime on `Info`/`Schema`: bare `@final` raises `InfoError`/`SchemaError` at first construction, deriving from a finalised class raises, and `SchemaError` is a `ValueError` where a caller may have been catching `TypeError`. Breaking. - #772 (with #790's docstring reword), #766, #759, #787, #794 (with #791's citation repoint), #792/#798 and #802 -- the remaining seven. Also re-ran the citation sweep against `origin/main` rather than the checkout. One stale line number fixed: the `httpv2.py` `header.length != 9` guard the `#692` entry calls out moved from `:562` to `:650` under #789 and #802. The preamble's "reaching #726" becomes #805, the new maximum reference. Verified unmoved on 4530424: `protocol.py:1411`, `schema/internet/ipv4.py:336`, `traceflow.py` 146/149/162/424, the four `:type:` fields in `engine.rst`, `reassembly.rst` and `traceflow.rst`, and `EXPECTED_FAILURES` at 43 entries. Carries the previous round's #651/#646 corrections unchanged. Two literals were reflowed so no ``literal`` wraps a line, which the generator's residual guard refuses. `changelog_md.py --check` exit 0; `test_changelog_md.py` 47 passed, 37 subtests.
`main` moved from 4530424 to 3cbdf89 while this PR sat open. Four are the new PRs merged in that window (#811-#814); the other four are older defects (#704, #723, #739, #743/#746) whose fixes had merged earlier but were never cited. Eight new bullets cover them, appended in merge order: - #704 -- `SystemdJournalExportBlock.post_process` skipped a binary field's trailing newline by reading to EOF, discarding every field behind it. - #723 -- the same block split entries on a bare `b'\n\n'`, shredding binary data that contains that byte pair; fixed alongside an independent trailing-separator/EOF ambiguity. - #739 -- five more registrars (`register_engine`/`_reassembly`/`_traceflow`, `register_dumper` x2) sat outside #718/#726's identity guard. - #743, #746 -- `pypcapfile`'s `IP.src`/`.dst` are dotted-decimal text, not packed bytes, and its frames need un-hexlifying before decoding; the two fixes are cross-dependent and landed together. - #805 -- `FieldBase.length`'s `struct.calcsize` on a negative resolved length raised a bare `struct.error`; now `ProtocolError`. Closes the follow-on #802's own entry filed as out of scope. - #796 -- thirteen `re.sub` sites under `pcapkit/vendor/` passed `re.MULTILINE` positionally as `count`, not as `flags=`. - #800 -- `httpv2._guess_version` now identifies a connection preface before parsing it, rather than by trial and error. Derived the gap by diffing `git log 73f09ae..origin/main` against `gh pr view --json state,mergedAt` for every candidate number, not from commit-subject text alone. `CHANGELOG.md` regenerated with `util/changelog_md.py`; `--check` exit 0 and `test_changelog_md.py`'s 47 tests pass. Sphinx's full-site build did not finish inside budget -- `pcapkit.const.reg`'s autodoc page is slow regardless of this change -- so verified instead with `docutils --report=1`, which parses the updated file with zero messages.
1 participant
make pylint,make mypy,make isort)make testpasses, and a test case covers the change — ran the targeted suite only (see below), not the fullmake testdocs/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 defectDescription of your pull request and other information
Ten code-keyed registrars warned on presence alone, so re-registering the exact same class read as "overwriting X with X" — worse than #710/#711, since nothing distinguished that no-op from a real collision. Added
register_protocol's identity guard (incumbent is not None and incumbent is not X) to all ten; message text unchanged.protocol.pyProtocolBase.registerinternet.pyInternet.registerlink.pyLink.registertransport.pyTransport.registersctp.pySCTP.registerframe.pyFrame.registerprotocols/misc/pcapng.pyPCAPNG.registerschema.pyEnumSchema.__init_subclass__(reachable via ordinary subclassing, not symmetry-only: a repeated or aliased member incode=[...]hits the same key twice withclson both sides)schema.pyEnumSchema.registerschema/misc/pcapng.pyOption.registerOption.registerwas the one holdout, now closed. It fans one registration out across a namespace dict-of-dicts rather than a single mapping, so it was left on presence-only with an explicit docstring admission that__init_subclass__'scode=[...]loop could reach it twice with the same class. It can, and the presence-only guard warned on the second call about the class overwriting itself. Same identity criterion as the other nine, adapted to check each namespacetargetsreaches via.get()rather thanin; docstring rewritten to match and drop the admission.Step 2 (#711's
id()disambiguation) — not applied. #711's own cross-review found theid()suffix defeats__warningregistry__dedup, with a nondeterministic delivered-warning count under GC pressure (199, 191, then 3 of 200 in one measurement). Once step 1 removes the dominant "X with X" cause, the residual case — two different classes sharing a repr — is mostly a dynamic-factory/test-class shape, not a real-usage concern. Paying that cost ten more times isn't worth it; the identity guard alone is the fix.Falsification: one same-object no-op test per site (9 total), each shown failing against the unfixed guard with the literal "overwriting X with X" text, passing after. Also fixed 6 pre-existing tests that used a same-object re-registration as their "overwrite" case, incl.
test_sibling_registries_still_warn_on_an_identical_re_registration, which pinned the old behaviour by name. Added a tenth test,test_a_repeated_code_in_the_declaration_list_stays_quiet, coveringschema.py's guard through ordinaryclass X(Base, code=[A, A])syntax and an enum-alias pair — the prior per-site test only called__init_subclass__()directly. ForOption.register: two more tests, both shown failing against the unfixed guard — acode=[b, b]registration of the same class (0 warnings expected, guard produced 1) and the same shape displacing a genuinely different class (1 warning expected, guard produced 2).Coverage (9 files, targeted suite): 3294→3301 stmts, miss and BrPart unchanged (180/97) — every new statement/branch fully exercised.
pylint/mypymessage counts unchanged in content (364/21) after retargeting two# type: ignorecomments mypy flagged as stale.schema/misc/pcapng.py(targetedtest_pcapng_unit.pyrun): 538 stmts/72 branches, 100% both before and after — the two new tests close a behavioural gap coverage numbers alone don't show.Could not verify: full
make test; CI (not run).Fixes #718.