refactor(const,protocols): reparent the last seven non-registry enums onto EnumLookup - #932
Conversation
JarryShaw
added
refactor
|
Two PR cross-references in the
The version #929 merged named no PR numbers and was not wrong; the numbers were added here. Being fixed on the same commit — either with the accurate attribution or by dropping the numbers. Independently verified on my side, against head
Opus cross-review in flight (author was Sonnet). CI green so far. Not ready to hand over. |
36f34bc to
f32fff0
Compare
|
NEEDS CHANGES at Blocking: The four extras are A new loud path, verified independently.
Refuted, and it was the author's: the "37 failures in a combined run, a pre-existing isolation artifact" claim does not reproduce — 296 passed / 0 failed on base, 321 / 0 on head, the +25 being exactly the new file. Nothing to alibi, and nothing this PR caused. Confirmed on the other claims: member counts identical for all seven (the Still unverified by anyone: |
JarryShaw
added
review: needs-changes
f32fff0 to
df8568a
Compare
… 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.
df8568a to
0a76861
Compare
|
Sphinx render verified — nothing this PR wrote is broken. Independent Opus check, explicit
Everything #932's own prose cites resolves to a real One line inside the rewritten The 16 are a real docs defect on Measured at |
JarryShaw
added
review: good-to-go
|
GOOD TO GO at The #933 reversal is correct and complete. Measured, head vs
Two corrections to my own earlier report of the author's claims. The " One gap this surfaced is filed as #935 rather than fixed here — the two kept overrides advertise the base's Unpublished and awaiting you. CI has one leg still running; I am not calling it merge-ready until that completes. |
|
I see one still using |
|
Ruling recorded, verbatim:
The finding is right. It is pre-existing, not introduced here. On Two things block me from just doing it, so I would rather ask than guess. First, Second, and more interesting: unlike So: do you want (a) a new
|
JarryShaw
added
review: needs-changes
|
go with (a) but i thought we already had that in other enumerations? how are we using |
|
Ruling recorded, verbatim:
Measured, and your conditional resolves to "the latter" — so by your own rule we keep the convention and change nothing. Census across
So Three pre-existing classes already pair The honest counter-evidence, so this is not cherry-picked: the four So: no change to
|
JarryShaw
added
review: good-to-go
JarryShaw
removed
the
review: good-to-go
…efault FastBindingAcknowledgmentStatus.get and IPv6AddressPrefixCode.get were re-parented onto EnumLookup by #932, which made them advertise the base's two-argument get(key, default) through inheritance while their own @staticmethod overrides still only accepted one -- a default argument raised TypeError instead of the base's own fallback. - Widen both signatures to key, default=NO_DEFAULT and resolve default through _value2member_map_, matching the base's own contract exactly rather than through the constructor: that constructor's own miss path, _missing_, raises loud, so probing a candidate default through it would set sys.tracebacklimit process-wide even on a miss the method goes on to answer quietly (the #362 hazard). - Remove both `# type: ignore[override] # pylint: disable=arguments-differ` suppressions the signature mismatch used to need; a narrow `# type: ignore[misc]` covers the four _value2member_map_ reads instead, since that attribute is generic over Self and mypy flags reading it through a bare class reference (no cls, staticmethod is kept) as ambiguous. - Add coverage in test_mh_unit.py proving the TypeError is gone and default is honoured exactly as the pure re-parents honour it, and in test_enum_lookup_reparent_930_unit.py pinning that the widened default path still raises quietly. Verified: mypy 321 errors/38 files and pylint 8.67/10 (exit 30) both unchanged against 3823758, with the two former [override] errors gone; all three target test modules pass individually.
…he base FastBindingAcknowledgmentStatus.get and IPv6AddressPrefixCode.get were re-parented onto EnumLookup by #932, which made them advertise the base's two-argument get(key, default) through inheritance while their own @staticmethod overrides still only accepted one. #935's first ruling widened both signatures to accept default; asked next "why must we have the two overrides tho? cant they directly fall back to the base class's?", the owner's final ruling went further, verbatim: "I prefer (2) directly" -- delete both overrides outright. - Delete both get() methods. Neither minted an alias (__members__ and list(cls) already agreed at 6 and 4), so "Backport support for original codes" was just the int-or-name dual resolution the base already provides; all 20 call sites (all in tests) passed only an int or a str. Both classes now inherit get/get_all from the base, the same as the five other re-parents. - Remove both `# type: ignore[override] # pylint: disable=arguments-differ` suppressions along with the methods -- absent now, not silenced. NO_DEFAULT and EnumKeyError drop out of the imports, unused once the methods that referenced them are gone. - Behaviour change, deliberate: the overrides branched on isinstance(key, int) and misrouted every other type through the name path, so get(None)/get(1.5) answered with a quiet EnumKeyError here against a loud EnumValueError on the other five. Deleting them makes all seven answer alike for the first time. - tests/corekit/test_enum_lookup_reparent_930_unit.py: drop the staticmethod pin in ReparentedBasesTests (nothing left to decorate); rename KeptOverrideQuietnessTests to InheritedQuietnessTests and PureReparentClassmethodTests to AllSevenInheritTheBareClassmethodTests, extended to all seven; add NonCanonicalKeyConvergenceTests pinning the get(None)/get(1.5) convergence. - tests/protocols/internet/test_mh_unit.py: repoint the default-widening test into one proving default now works uniformly across all four of this module's EnumLookup classes through the single inherited method. Verified: mypy 321 errors/38 files and pylint 8.67/10 (exit 30) both unchanged against 3823758 (the two mh.py pylint findings that do disappear are the two now-deleted f-string-eligible raises); the two [override] errors are absent rather than suppressed. All three target test modules pass individually; the new convergence and uniformity tests both fail against 3823758 and pass here.
…he base FastBindingAcknowledgmentStatus.get and IPv6AddressPrefixCode.get were re-parented onto EnumLookup by #932, which made them advertise the base's two-argument get(key, default) through inheritance while their own @staticmethod overrides still only accepted one. #935's first ruling widened both signatures to accept default; asked next "why must we have the two overrides tho? cant they directly fall back to the base class's?", the owner's final ruling went further, verbatim: "I prefer (2) directly" -- delete both overrides outright. - Delete both get() methods. Neither minted an alias (__members__ and list(cls) already agreed at 6 and 4), so "Backport support for original codes" was just the int-or-name dual resolution the base already provides; all 20 call sites (all in tests) passed only an int or a str. Both classes now inherit get/get_all from the base, the same as the five other re-parents. - Remove both `# type: ignore[override] # pylint: disable=arguments-differ` suppressions along with the methods -- absent now, not silenced. NO_DEFAULT and EnumKeyError drop out of the imports, unused once the methods that referenced them are gone. - Behaviour change, deliberate: the overrides branched on isinstance(key, int) and misrouted every other type through the name path, so get(None)/get(1.5) answered with a quiet EnumKeyError here against a loud EnumValueError on the other five. Deleting them makes all seven answer alike for the first time. - tests/corekit/test_enum_lookup_reparent_930_unit.py: drop the staticmethod pin in ReparentedBasesTests (nothing left to decorate); rename KeptOverrideQuietnessTests to InheritedQuietnessTests and PureReparentClassmethodTests to AllSevenInheritTheBareClassmethodTests, extended to all seven; add NonCanonicalKeyConvergenceTests pinning the get(None)/get(1.5) convergence. - tests/protocols/internet/test_mh_unit.py: repoint the default-widening test into one proving default now works uniformly across all four of this module's EnumLookup classes through the single inherited method. Verified: mypy 321 errors/38 files and pylint 8.67/10 (exit 30) both unchanged against 3823758 (the two mh.py pylint findings that do disappear are the two now-deleted f-string-eligible raises); the two [override] errors are absent rather than suppressed. All three target test modules pass individually; the new convergence and uniformity tests both fail against 3823758 and pass here.
…efault (#940) FastBindingAcknowledgmentStatus.get and IPv6AddressPrefixCode.get were re-parented onto EnumLookup by #932, which made them advertise the base's two-argument get(key, default) through inheritance while their own @staticmethod overrides still only accepted one. #935's first ruling widened both signatures to accept default; asked next "why must we have the two overrides tho? cant they directly fall back to the base class's?", the owner's final ruling went further, verbatim: "I prefer (2) directly" -- delete both overrides outright. - Delete both get() methods. Neither minted an alias (__members__ and list(cls) already agreed at 6 and 4), so "Backport support for original codes" was just the int-or-name dual resolution the base already provides; all 20 call sites (all in tests) passed only an int or a str. Both classes now inherit get/get_all from the base, the same as the five other re-parents. - Remove both `# type: ignore[override] # pylint: disable=arguments-differ` suppressions along with the methods -- absent now, not silenced. NO_DEFAULT and EnumKeyError drop out of the imports, unused once the methods that referenced them are gone. - Behaviour change, deliberate: the overrides branched on isinstance(key, int) and misrouted every other type through the name path, so get(None)/get(1.5) answered with a quiet EnumKeyError here against a loud EnumValueError on the other five. Deleting them makes all seven answer alike for the first time. - tests/corekit/test_enum_lookup_reparent_930_unit.py: drop the staticmethod pin in ReparentedBasesTests (nothing left to decorate); rename KeptOverrideQuietnessTests to InheritedQuietnessTests and PureReparentClassmethodTests to AllSevenInheritTheBareClassmethodTests, extended to all seven; add NonCanonicalKeyConvergenceTests pinning the get(None)/get(1.5) convergence. - tests/protocols/internet/test_mh_unit.py: repoint the default-widening test into one proving default now works uniformly across all four of this module's EnumLookup classes through the single inherited method. Verified: mypy 321 errors/38 files and pylint 8.67/10 (exit 30) both unchanged against 3823758 (the two mh.py pylint findings that do disappear are the two now-deleted f-string-eligible raises); the two [override] errors are absent rather than suppressed. All three target test modules pass individually; the new convergence and uniformity tests both fail against 3823758 and pass here.
1 participant
Please follow the guide below
make pylint,make mypy,make isort)make testpasses, and a test case covers the changedocs/source/changelog/and regeneratedCHANGELOG.md, if the change is user-visible -- N/A -- changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657What is the purpose of your pull request?
refactor— changes neither behaviour nor performanceDescription of your pull request and other information
Closes #930. Finishes #877's phase 2 -- #921 re-parented 17 of the 24 non-registry enumerations onto
EnumLookupand deliberately left the remaining seven alone because the files holding them were still being edited by other pull requests:pcapkit.const.ftp.command(CommandType,ConformanceRequirement) and its vendor template, both touched by #913; andpcapkit.protocols.internet.esp(ESPStatus) pluspcapkit.protocols.internet.mh(FastBindingAcknowledgmentStatus,IPv6AddressPrefixCode,LMAAddressCode,LocalizedRoutingStatus), both files touched by #924. Both have since merged. (An earlier draft of this description and ofconventions.rstnamed #904 for theesp.py/mh.pyhalf; verified againstgh api repos/JarryShaw/PyPCAPKit/pulls/904/files, #904 never touches either file -- #924 does. Corrected in both places.)Re-derived the census myself first (
pkgutil.walk_packagesover every importablepcapkit.*module, vendor templates excluded, filtering onissubclass(cls, EnumLookup)) and got exactly the same seven the issue names -- no extras, no omissions.FastBindingAcknowledgmentStatusandIPv6AddressPrefixCodealready had their ownget, both still astaticmethod-- kept untouched per the issue's own brief, since neither callssuper().get(...)(so thestaticmethod-delegating-to-classmethodRuntimeErrortrap #921 hit never applies here) and #923 already converted their name-miss to the in-libraryEnumKeyError, exactly the shape the base now uses. Keeping the@staticmethoddoes tripmypy's[override]andpylint'sarguments-differagainst the base'sclassmethodsignature; both are suppressed (# type: ignore[override] # pylint: disable=arguments-differ, matching existing precedent inpcapkit/vendor/reg/apptype/apptype.py) rather than resolved by widening the signature, since that would mean touching the kept logic. The other five (CommandType,ConformanceRequirement,ESPStatus,LocalizedRoutingStatus,LMAAddressCode) are pure re-parents with nogetof their own to reconcile.One new public-API surface is worth calling out on its own:
get_all, inherited fromEnumLookupfor the first time onFastBindingAcknowledgmentStatus/IPv6AddressPrefixCode, calls their keptgetinternally, so a name miss reached throughget_allnow reaches whatevergetitself does. I asked separately whether these two overrides should adopt the base's ownquiet=True. The owner's first answer was no (settled on #933); four minutes later he corrected himself, verbatim: "Oh wait. I meant, they should follow house convention and not to be loud." So both overrides do now adoptquiet=True-- a real behaviour change, not merely new reachability: pre-fix, a name miss on either class logged once atCRITICALand setsys.tracebacklimit = 0process-wide; post-fix, it does neither, ongetor on the newly-inheritedget_all. Both overrides' docstrings and the renamedKeptOverrideQuietnessTeststest class cite #933 for the converged, quiet answer.Member-table sizes are unchanged for all seven (verified
len(__members__)andlen(list(cls))before and after, plus a runtime battery of lookups that doesn't grow any of them).pcapkit/vendor/ftp/command.py(the crawler template) is updated in lockstep withpcapkit/const/ftp/command.py, matching house rule that a generated registry's shape lives in the crawler.Also brought
docs/source/contributing/conventions.rstand its owntests/project/test_conventions_doc_claims.pyin line, since #929 merged in the interim and both said seven remained outside the hierarchy. Phase 2 is now 24 of 24, zero enumerations outsideEnumLookup-- both doc-claims tests still derive their figures from a runtime walk rather than a hardcoded number, so they keep pinning the fact rather than a snapshot of it.test_the_page_names_every_enumeration_outside_the_hierarchyis now vacuous (its loop has nothing left to iterate) and says so explicitly viaskipTestrather than passing silently.New test file
tests/corekit/test_enum_lookup_reparent_930_unit.pypins: the base-tuple/MRO change and member-table sizes for all seven, thatgetresolves by name and by value, what a miss raises (KeyError-derived for a name,ValueError-derived for a value), that the two kept overrides are stillstaticmethodwhile the five pure re-parents inherit the baseclassmethod, that a battery of lookups mints nothing, the zero-outside census itself, and theget/get_allquietness described above. Measured against the three source files reverted toaf2324522(plainunittest, eachsubTestresolved back to its parent method viagetattr(test, 'test_case', test)-- without that resolution asubTest-only failure is recorded againstunittest.case._SubTestand its parent method misreads as passing): 22 of its 27 test methods fail or error; five hold. One is a scaffolding guard (ZeroRemainOutsideEnumLookupTests.test_the_walk_found_something_to_count). The other four hold becauseFastBindingAcknowledgmentStatus/IPv6AddressPrefixCodealready had a workinggetbefore this change, so their name/value lookup and failed-lookup behaviour -- not their loudness, see below -- is unaffected by the re-parent (FailedLookupTests/GetByNameAndValueTests×2 each). What does fail for both classes is their ownReparentedBasesTestsmethod and bothKeptOverrideQuietnessTestsmethods: the base-tuple change,get_all's existence, and the switch from loud to quiet are each real regardless of whetherget's name/value resolution already worked. (This count moved twice during review. An earlier revision undercounted it as one hold out of 25/34; a cross-review caught that and it was re-measured to 21/27 with six holding. The #933 reversal above then flippedtest_get_is_now_quiet_on_both_classesfrom holding to failing -- it used to pin loud, which already matched the reverted tree; it now pins quiet, which the reverted tree's genuinely-loudgetfails -- landing on the 22/27 five-hold figure quoted here, which is also what the file's own module docstring now says.)Three pre-existing tests made stale claims this change falsifies, fixed in the same commit:
test_const_enum_get.py'sEXPECTED_WITHOUT_AN_INTEGER_DEFAULTexclusion set (now empty,CommandType/ConformanceRequirementmoved into the main sweep),test_const_ftp_featcode_case_903_unit.py's literal import-line assertion (now includesEnumLookup), andtest_mh_unit.py'shasattr(enum_cls, 'get')assertion forLocalizedRoutingStatus/LMAAddressCode(nowTrue, inherited from the base rather than absent).Linters, re-measured against a freshly-extracted baseline of the four touched source files from
af2324522rather than summarised:mypyreports the same 17 pre-existing errors either side (none in the[override]/arguments-differcategory the two suppressions target);pylintreports 391 messages either side, identical message-for-message bar a one-line shift from my own added import (confirmed by diffing the sorted message lists);isort -l100 -ppcapkitis clean (exit 0, no diff) on all four files, matchingtests/project/test_isort_clean.py.Tests: every batch I ran individually is green --
tests/const/test_const_enum_get.py,test_const_enum_no_mint.py,test_const_enum_builtin_parity.py,test_const_enum_lookup.py,test_const_ftp_featcode_case_903_unit.py(144 tests);tests/dumpkit/test_nameless_enum_rendering_unit.pyplustests/protocols/internet/test_esp_unit.py/test_mh_unit.py(81 tests);tests/corekit/test_sentinel_exports_unit.py,test_enum_lookup_reparent_877_unit.py, the newtest_enum_lookup_reparent_930_unit.py,tests/project/test_conventions_doc_claims.py, andtest_isort_clean.py(98 tests, 1 skipped as above). I previously reported that combining all thirteen of those files into onepython -m unittestinvocation threw 37 failures, and attributed it to a "pre-existingsys.modules-purge isolation artifact" without having reproduced that mechanism -- a cross-review couldn't reproduce it and called that out correctly. Re-measured: it does reproduce, deterministically, under plainpython -m unittestwith this exact 13-file list. That is pre-existing on cleanmainunder bareunittest, measured -- not merely a pytest-vs-unittest difference, which is what I said the first time and which undersold it: the 24tests/corekit/test_*.pyfiles common to both trees, run under bareunitteston an unmodifiedaf2324522checkout (git worktree add --detachto a scratch path, removed afterward), throw their own cross-file failures independent of anything in this PR --ran=370 failure_entries=5 error_entries=0 distinct_bad=3, all three insidetest_sentinel_exports_unit.py(NoValueIsTheDocumentedFieldDefaultTests×2,SentinelExportTests.test_star_import_hands_back_the_canonical_object) -- which I reproduced independently rather than took on faith. The identical 13 files on this branch underpytest, which is whatmake testactually invokes, are clean:322 passed, 1 skipped, 0 failed, matching a cross-review's own result. So: pre-existing onmainunder bareunittest; clean under the runner this project's own tooling uses. I withdraw both the "isolation artifact" and the "runner-specific" framings as incomplete, and replace them with this directly-reproduced pair of findings.