refactor: retire transitional PROPERTY_MAPPING parser seams (#798) - #866
Merged
Conversation
… decision (#798) Adds option_key_is_present as the single allow_explicit_none presence decision (six inline copies replaced) and removes _is_context_parameter, _is_strict_validation, _get_element_validator, the _extract_property_values alias, and the mixin _build_effective_options pass-through. Tests migrated to the canonical PropertySpec fields and public methods.
…#798) check_required_when no longer contains a raise out of framework-owned build_effective_options as a non-match. No user callback runs inside the build, so containment there could only hide internal parser defects or actionable configuration errors. User-supplied validator, guard, and predicate containment is unchanged.
…ing diagnostics (#798) The engine now records structured rejection reasons (strict value rejections, strict match_guard rejections, name-path missing-required-option reasons) while the real match pass runs, and renders the resolution-failure report from that capture. _strict_validation_rejection_reason stays as a standalone compatibility facade producing identical messages, but the engine no longer calls it, so user validators and parser logic run exactly once per candidate.
Review fix: the recorder window now opens per candidate match call and the harvested reason is keyed by the candidate class object, so two same-named feature groups from different modules keep their own rejection reasons instead of collapsing into one name-keyed slot. Also drops the unused feature parameter from _value_rejection_reason and documents the deliberately kept string-path containment asymmetry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Retires the transitional PROPERTY_MAPPING parser seams left by the hardening epic (board issue os-005, migrated from #798, phase 7 of the epic tracked in #761).
What changed
Trivial private wrappers removed.
FeatureChainParser._is_context_parameter,_is_strict_validation,_get_element_validator, and the_extract_property_valuesalias are gone; production code and tests read the canonicalPropertySpecfields and call the publicextract_property_values(). The mixin's test-only_build_effective_optionspass-through is gone too (no public compatibility contract required it).Presence semantics centralized. The
allow_explicit_nonepresence decision existed as six inline copies (four in the parser, one in the mixin, one infeature_group.py). They are replaced by one shared helper,option_key_is_present, keeping the behavior matrix from feat: opt-in way to distinguish explicit None from an absent option #768: an opted-in explicitNonecounts as present, a flagless present-as-Nonedoes not. Equivalence at every site follows fromOptions.get/__contains__semantics and is pinned by tests.Framework-owned exception containment narrowed.
check_required_whenno longer swallows a raise out of framework-ownedbuild_effective_optionsas a non-match. No user callback runs inside the build, so such a raise is a framework defect (or a user configuration error carrying actionable guidance) and now surfaces. Containment of user-supplied validators, guards, andrequired_whenpredicates is unchanged.Diagnostic replay removed from the engine. The resolution-failure report used to re-run
_strict_validation_rejection_reasonper candidate, re-executing user validators and parser logic a second time. The engine now opens a recording window around each candidate's real match call and captures the structured rejection reason as it happens (strict value rejections, strictmatch_guardrejections, name-path missing-required-option reasons), keyed by the candidate class object so same-named classes from different modules keep their own reasons._strict_validation_rejection_reasonremains as a standalone diagnostic facade producing identical messages, but the engine never calls it: user callbacks run exactly once per candidate per resolution.No public API is removed; all deleted names were underscore-private transitional seams.
attribution/ATTRIBUTION.mdpicked up the current dependency resolution (defusedxml, nltk 3.10.0) per the tox regeneration rule.Review
Two independent reviews (Claude Opus and codex) ran on the diff. Both flagged the same defect in the first recorder implementation: name-keyed, first-wins recording collapsed same-named candidate classes. Fixed by the per-candidate recording window with class-object attribution, with a regression test. Remaining accepted nits: dropped an unused parameter and documented the deliberately kept string-path containment asymmetry (a config error raised while merging name bindings stays contained on the string path, while the
required_whenpath surfaces it).Testing
Full
toxgreen twice (after the main change and after the review fixes): 7160 passed, 170 skipped, ruff format/check, license allowlist,mypy --strict(883 files), bandit.