fix: take render_resolution_failure from its owning module - #914
Merged
Conversation
The renderer split gave resolution_failure_renderer ownership of render_resolution_failure, and added a sweep that fails any call site importing a name the matcher does not own. The raising-matcher containment test branched before that sweep existed, so it still took the name from identify_feature_group and turned main red on merge.
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.
mainis red at410ff53d: all 15 CI jobs fail on the same test.Cause
A merge race between two changes that were each green on their own branch:
identify_feature_groupbeing a re-export facade and added the sweep that fails any call site importing a name the matcher does not own.render_resolution_failureis owned byresolution_failure_renderer.test_raising_matcher_containment.py, whose branch predated that sweep, so it still took the name fromidentify_feature_group.They merged 51 seconds apart. The import still resolves at runtime, because the matcher imports the renderer for its own use in
evaluate_and_render, which is why only the sweep fails and not the containment test itself.Fix
Take
render_resolution_failurefrom its owning module.IdentifyFeatureGroupClassstays with the matcher, which owns it.This clears two gates, not one:
mypy --strictalso rejected the old form as an implicit re-export (Module "mloda.core.prepare.identify_feature_group" does not explicitly export attribute "render_resolution_failure"). Only the pytest sweep showed up in CI because pytest runs first.Moving the name into
MATCHER_KEPT_NAMESinstead would not work:test_kept_names_stay_defined_in_the_matcher_moduleasserts each kept name's__module__is the matcher, andRENDERER_NAMESalready claims this one for the renderer.Test
Test-only change, one import line. The failing sweep is itself the regression test, so no new test is added. Full
toxgreen locally: 7584 passed, 170 skipped, plus ruff, licenses,mypy --strict, bandit.