fix(corekit): reparent the unblocked non-registry enums onto EnumLookup (#877) - #921
Conversation
JarryShaw
added
fix
db5953a to
21c9b28
Compare
|
Cross-review on Opus returned NEEDS CHANGES (author was Sonnet). Both findings were prose, both are now fixed — new head The code was sound; two comments asserted something measurement refutes. I re-derived both myself on What was wrong. The delegation is exception-compatible for every key the signature admits (
No caller can reach any of it: the only live sites are Also corrected: the pre-fix failure count is 31 Verified after the amend: 51 tests across |
|
GOOD TO GO at head The two load-bearing claims, measured independently:
Worth recording from the reviewer, correcting its own earlier framing: the template is a Behaviour is unchanged from One item remains UNVERIFIED and is not blocking: the Unpublished and unmerged — yours to merge. Note the merge-order constraint recorded on #922. |
JarryShaw
added
review: good-to-go
|
We may want to revise if their overrides after subclassing |
|
Ruling recorded, verbatim:
Measured before answering, on
So neither override is redundant — but each is down to a small, nameable job. Deleting either would be a public behaviour change, not a cleanup: a name miss would start raising Which leaves a real design question, and it is yours: is the Labelled |
JarryShaw
added
the
needs: decision
|
Either |
…up (#877) Phase 2 of #877, the 11 classes across 8 files not held by #913/#904: TransportProtocol, FinalisedState, Completion, ftp.Type, httpv1.Type, Criticality, PDUKind, PacketDirection, PacketReception, WireGuardKeyLabel, and FrameType.Flags (carrying its 6 per-frame subclasses transitively). Each now mixes in EnumLookup ahead of its enum base for the shared get/get_all contract. TransportProtocol and Criticality already had their own get, both as a staticmethod against EnumLookup.get's classmethod (the #908/#915 trap). Both are now classmethods delegating to super().get(), keeping only what the base does not reproduce -- TransportProtocol's case-fold and no-mint refusal, Criticality's case-sensitive miss -- each re-raised as the ValueError callers already depend on rather than the base's KeyError. Each gained a default parameter forwarded to the base, since dropping one the base declares is a real classmethod-override violation under mypy. Updated test_const_enum_get.py's exclusion set for TransportProtocol's new default, and added test_enum_lookup_reparent_877_unit.py pinning the re-parenting, both preserved overrides, and no member-table growth.
21c9b28 to
3e7d73d
Compare
|
Ruling recorded, verbatim:
Measured on Python 3.14.7, so the shape the ruling selects is concrete: Two consequences, and one gap I did not expect:
Filed as #923 ( Removing Also rebased onto |
JarryShaw
removed
the
needs: decision
|
So the overloads are still legit? Or they will be fixe/updated in #923? |
|
Both: legitimate as shipped here, then reduced in #923. Measured rather than assumed. Legitimate now. Each override still does something the inherited base does not, so nothing in this PR ships dead code:
#923 removes the conversion, and that is what changes each of them. Under the ruling — stdlib shape,
The only thing lost by deleting it is the narrower One thing I went looking for and did not find, so it is not a defect to fix: I expected the So: merge this as-is, and #923 does the reduction. I have recorded the above in #923's scope so the two-into-one shrink is not rediscovered there. |
JarryShaw
removed
the
review: good-to-go
… onto EnumLookup (#930) Finishes #877's phase 2, which #921 left seven classes out of because their files were held by #913/#904 at the time: CommandType and ConformanceRequirement (const/ftp/command.py), ESPStatus (protocols/internet/esp.py), and FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LMAAddressCode and LocalizedRoutingStatus (protocols/internet/mh.py). Both blockers have since merged. Each now mixes in EnumLookup ahead of its enum base; member-table sizes are unchanged. FastBindingAcknowledgmentStatus and IPv6AddressPrefixCode kept their own get, still a staticmethod that never calls super() -- left untouched, since #923's EnumKeyError name-miss conversion already matches the base's shape. mypy's [override] and pylint's arguments-differ against the kept decorator are suppressed rather than resolved by widening it. The other five are pure re-parents. Brought conventions.rst and its own doc-claims test in line with #929, which merged in the interim: phase 2 is now 24 of 24, zero enumerations outside the hierarchy. Added test_enum_lookup_reparent_930_unit.py pinning the re-parenting, the kept overrides, no growth, and the zero-outside census; fixed three tests whose claims this change made stale (test_const_enum_get, test_const_ftp_featcode_case_903_unit, test_mh_unit). Build: mypy/pylint/isort clean against baseline; affected test files pass.
|
Correction to this PR's description, for the record. It says Measured from the API: #904 touched neither file. Its file list is The This does not change anything this PR did — the 11 classes it re-parented were the right set, and the seven it deferred were genuinely contended. Surfaced while reviewing #932, which finished those seven and had inherited the same misattribution from here. |
… onto EnumLookup (#930) Finishes #877's phase 2, which #921 left seven classes out of because their files were held by #913/#904 at the time: CommandType and ConformanceRequirement (const/ftp/command.py), ESPStatus (protocols/internet/esp.py), and FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LMAAddressCode and LocalizedRoutingStatus (protocols/internet/mh.py). Both blockers have since merged. Each now mixes in EnumLookup ahead of its enum base; member-table sizes are unchanged. FastBindingAcknowledgmentStatus and IPv6AddressPrefixCode kept their own get, still a staticmethod that never calls super() -- left untouched, since #923's EnumKeyError name-miss conversion already matches the base's shape. mypy's [override] and pylint's arguments-differ against the kept decorator are suppressed rather than resolved by widening it. The other five are pure re-parents. Brought conventions.rst and its own doc-claims test in line with #929, which merged in the interim: phase 2 is now 24 of 24, zero enumerations outside the hierarchy. Added test_enum_lookup_reparent_930_unit.py pinning the re-parenting, the kept overrides, no growth, and the zero-outside census; fixed three tests whose claims this change made stale (test_const_enum_get, test_const_ftp_featcode_case_903_unit, test_mh_unit). Build: mypy/pylint/isort clean against baseline; affected test files pass.
… onto EnumLookup (#930) Finishes #877's phase 2, which #921 left seven classes out of because their files were held by #913/#904 at the time: CommandType and ConformanceRequirement (const/ftp/command.py), ESPStatus (protocols/internet/esp.py), and FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LMAAddressCode and LocalizedRoutingStatus (protocols/internet/mh.py). Both blockers have since merged. Each now mixes in EnumLookup ahead of its enum base; member-table sizes are unchanged. FastBindingAcknowledgmentStatus and IPv6AddressPrefixCode kept their own get, still a staticmethod that never calls super() -- left untouched, since #923's EnumKeyError name-miss conversion already matches the base's shape. mypy's [override] and pylint's arguments-differ against the kept decorator are suppressed rather than resolved by widening it. The other five are pure re-parents. Brought conventions.rst and its own doc-claims test in line with #929, which merged in the interim: phase 2 is now 24 of 24, zero enumerations outside the hierarchy. Added test_enum_lookup_reparent_930_unit.py pinning the re-parenting, the kept overrides, no growth, and the zero-outside census; fixed three tests whose claims this change made stale (test_const_enum_get, test_const_ftp_featcode_case_903_unit, test_mh_unit). Build: mypy/pylint/isort clean against baseline; affected test files pass.
… onto EnumLookup (#932) Finishes #877's phase 2, which #921 left seven classes out of because their files were held by #913/#904 at the time: CommandType and ConformanceRequirement (const/ftp/command.py), ESPStatus (protocols/internet/esp.py), and FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LMAAddressCode and LocalizedRoutingStatus (protocols/internet/mh.py). Both blockers have since merged. Each now mixes in EnumLookup ahead of its enum base; member-table sizes are unchanged. FastBindingAcknowledgmentStatus and IPv6AddressPrefixCode kept their own get, still a staticmethod that never calls super() -- left untouched, since #923's EnumKeyError name-miss conversion already matches the base's shape. mypy's [override] and pylint's arguments-differ against the kept decorator are suppressed rather than resolved by widening it. The other five are pure re-parents. Brought conventions.rst and its own doc-claims test in line with #929, which merged in the interim: phase 2 is now 24 of 24, zero enumerations outside the hierarchy. Added test_enum_lookup_reparent_930_unit.py pinning the re-parenting, the kept overrides, no growth, and the zero-outside census; fixed three tests whose claims this change made stale (test_const_enum_get, test_const_ftp_featcode_case_903_unit, test_mh_unit). Build: mypy/pylint/isort clean against baseline; affected test files pass.
…ting them The last tranche of #987, plus #995. These files are generated, so every change is made twice: in the vendor template and identically in the const file it renders. The template holds the prose verbatim with {NAME}/{DOCS} placeholders, so the next crawler run reproduces it. No crawler was run. Scope: 22 italic-quoted spans across 7 files, of which 16 are rulings and converted. Six are not and stay -- all of them RFC 5797 and RFC 2389 text on FEAT-code case sensitivity. The italic pattern also missed 14 straight-quoted spans in the apptype pair, 7 per side, most wrapping a backticked vertical bar; those are converted too. - #877 was revised twice and the prose stated the middle revision. The final ruling is RFC-directed: an enum treats its values as case-insensitive where the RFC states they are, and as case-sensitive otherwise. The earlier form, which also allowed a fold where it logically made sense, is out. - #860's ruling is narrowed to what it says. The prose called it the ruling for the whole family, but the comment reads "for all three" and names FEATCode, Command and Method. AppType is never named in it, and the AppType work is #874 under #860, so the prose now says the family follows a ruling given for those three rather than that it was given for the family. - #860's FEAT-value question was answered conditionally, on whether it matched the approach already in use; that conditional is restored. - The #921 exception ruling had lost its second clause -- that the exception comes from pcapkit.utilities.exceptions rather than being a builtin. - #995: vendor/ipx/socket.py credited a ruling with "a real ownership fact", which is in no maintainer comment. The real reason, #847 at 13:15:36Z, is that a proprietary protocol may expose no name of its own. That comment is module documentation for UNASSIGNED_RANGE_NAMES and is not emitted into the const file, so only the template changed. - Re-flowing wrapped one inline literal that the base had whole, and left five orphan tails. All six are closed. Prose only, and proven against the one risk that matters in a generated file. Importing both trees gives a byte-identical sha256 over every name and value for the seven enums the three touched const files define -- 127 members, 126 iterable -- and the generated rST table rows are identical at 603 distinct of 707. Token sequences match per file with comments and NL dropped, masking FSTRING_MIDDLE as well as STRING since the templates are f-strings and their prose tokenises as the former. The over-95 line set is unchanged in every file. Closes #995.
…ting them The last tranche of #987, plus #995. These files are generated, so every change is made twice: in the vendor template and identically in the const file it renders. The template holds the prose verbatim with {NAME}/{DOCS} placeholders, so the next crawler run reproduces it. No crawler was run. Scope: 22 italic-quoted spans across 7 files, of which 16 are rulings and converted. Six are not and stay -- all of them RFC 5797 and RFC 2389 text on FEAT-code case sensitivity. The italic pattern also missed 14 straight-quoted spans in the apptype pair, 7 per side, most wrapping a backticked vertical bar; those are converted too. - #877 was revised twice and the prose stated the middle revision. The final ruling is RFC-directed: an enum treats its values as case-insensitive where the RFC states they are, and as case-sensitive otherwise. The earlier form, which also allowed a fold where it logically made sense, is out. - #860's ruling is narrowed to what it says. The prose called it the ruling for the whole family, but the comment reads "for all three" and names FEATCode, Command and Method. AppType is never named in it, and the AppType work is #874 under #860, so the prose now says the family follows a ruling given for those three rather than that it was given for the family. - #860's FEAT-value question was answered conditionally, on whether it matched the approach already in use; that conditional is restored. - The #921 exception ruling had lost its second clause -- that the exception comes from pcapkit.utilities.exceptions rather than being a builtin. - #995: vendor/ipx/socket.py credited a ruling with "a real ownership fact", which is in no maintainer comment. The real reason, #847 at 13:15:36Z, is that a proprietary protocol may expose no name of its own. That comment is module documentation for UNASSIGNED_RANGE_NAMES and is not emitted into the const file, so only the template changed. - The exception ruling was credited to issue #923, which carries no maintainer comment at all -- its own body attributes the ruling to the review of #877's implementation. So the prose now credits the ruling to that review and #923 with the implementation, keeping the citation in issue form as docs/source/contributing/conventions/documentation.rst requires. - Re-flowing wrapped one inline literal that the base had whole, and left five orphan tails. All six are closed. Prose only, and proven against the one risk that matters in a generated file. Importing both trees gives a byte-identical sha256 over every name and value for the seven enums the three touched const files define -- 127 members, 126 iterable -- and the generated rST table rows are identical at 603 distinct of 707. Token sequences match per file with comments and NL dropped, masking FSTRING_MIDDLE as well as STRING since the templates are f-strings and their prose tokenises as the former. The over-95 line set is unchanged in every file. Closes #995.
…issue (#719) - Nine citations in tests/ called a pull request "GitHub issue" (#921, #764, #906, #815, #936, #721, #501, #983) or lumped PR #428 in with issue #425; each now names the right kind. - test_dispatch_default_resolution_unit: issue #425 reported the registry leak and PR #428 fixed it, so the sentence says "reported" and "fixed" instead of crediting the issue with the fix. - Prose only: docstrings and comments, no assertion or logic touched.
1 participant
Please follow the guide below
make pylint,make mypy,make isort)make testpasses, and a test case covers the changeWhat is the purpose of your pull request?
fix-- corrects a defectDescription of your pull request and other information
Phase 2 of #877, the owner's ruling to reparent every non-registry enum onto
EnumLookup. This covers the 11 classes across 8 files not held by #913 or #904:TransportProtocol,FinalisedState,Completion,ftp.Type,httpv1.Type,Criticality,PDUKind,PacketDirection,PacketReception,WireGuardKeyLabel, andFrameType.Flags(which carries its 6 concrete per-frame subclasses transitively -- verified at runtime, not assumed).TransportProtocolandCriticalityalready defined their ownget, both as astaticmethodagainstEnumLookup.get'sclassmethod-- the exact trap #908 hit and #915 fixed. Both are nowclassmethods delegating tosuper().get(), keeping only the behaviour the base doesn't reproduce (case-folding and the PR #836 no-mint refusal forTransportProtocol; case-sensitivity forCriticality), each still raising theValueErrorcallers already depend on rather than the base'sKeyError. Each gained adefaultparameter forwarded verbatim to the base, since dropping an optional parameter the base declares is a realclassmethod-override violation under mypy.pcapkit/const/ftp/command.py(CommandType,ConformanceRequirement) andpcapkit/protocols/internet/{esp,mh}.pyare untouched, still held by #913/#904 respectively.