refactor: stop identify_feature_group being a re-export facade - #908
Merged
Conversation
…module The identify_feature_group split kept every pre-split import working, so plugin_docs still reached the renderer and the result types through the matcher. Point each call site at the module that owns the name and drop the TROUBLESHOOTING_URL and scope_callout re-exports, which nothing consumed through the matcher.
Two reviews found the surface decision could not be re-derived from the comments: the stated criterion explained importing rather than re-exporting, and nothing said why the result types stay a block. Also pin that every public renderer name is classified, so a new one cannot land unpinned.
The compatibility block guarded an API no release exposes: at 0.10.0 the module had four top-level names and no __all__, so 13 of the 15 names it re-exported have never shipped. Delete __all__ outright, since a module always exports what it defines, and point all 26 call sites at the owning module. A static sweep now fails any import of a borrowed name from the matcher, replacing three hand-maintained name tuples.
Deriving the repo root from the imported module made the sweep silently vacuous under the sdist-installed env, where the matcher resolves to site-packages and the tests tree is simply absent. Derive it from the test file, add docs/ (its marimo notebooks are outside mypy and F401), and gate on the bare module segment so an unusual dotted spelling stays in scope.
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.
Follow-up to #891, which split
identify_feature_group.pyintoresolution_types.py<-resolution_failure_renderer.py<- the matcher. That PR deliberately kept every pre-split import working via an__all__re-export block, so no call site had to move. This removes that block. Behavior-preserving.Why remove it rather than tidy it
The block was a compatibility layer for an API that no release exposes. At tag
0.10.0, still the latest release,mloda/core/prepare/identify_feature_group.pycontained four top-level names (split_frameworks_by_capability, since deleted;matches_feature_group_scope;_scope_callout, private;IdentifyFeatureGroupClass) and declared no__all__at all. Neither sibling module existed. So 13 of the 15 names it re-exported have never shipped, andmloda.core.*is internal regardless: the public facades aremloda.user,mloda.providerandmloda.steward, which import onlyFeatureResolutionError,ResolutionDiagnosisandResolutionRecord.Removing
__all__also restores the released behavior rather than changing it: 0.10.0 had none.Changes
__all__deleted outright rather than trimmed. Undermypy --strict(--no-implicit-reexport) a module always exports what it defines, so__all__there only ever re-exported names it merely imports. With it gone, an import kept for a downstream consumer is just an unused import and ruff F401 rejects it, which is what keeps the facade from returning one name at a time. That is now noted in the import block.resolution_typesfor the result dataclasses andPARTIAL_RECORDS_CAP,resolution_failure_rendererfor the rendering names, the matcher forFeatureResolutionError,ComputeFrameworkPinError,IdentifyFeatureGroupClass,matches_feature_group_scope,evaluate_and_render,resolve_or_raise. The three facades keep their own__all__unchanged; only the source of two names moves.ResolutionDiagnosisalso leaves the matcher's imports: it was imported solely to feed__all__and the module body never used it.mloda/,mloda_plugins/,tests/anddocs/.mypyis the primary enforcement and catches every static evasion; the sweep covers the case mypy goes quiet on, someone re-adding__all__and routing call sites back through it.test_identify_feature_group_failure_renderer.pytotest_resolution_failure_renderer.pyand corrected two docstrings that placed_render_multipleandPARTIAL_RECORDS_CAPin the matcher.docs/docs/in_depth/mloda-api.mdno longer advertisesEvaluationResultas importable from the matcher.Verification
toxgreen: 7535 passed, 170 skipped, ruff, license allowlist,mypy --strictover 901 files, bandit. Test count is net -1 because the pin file went from 9 tests to 8; no skip-count change.Two independent deep reviews ran against the full diff. Findings worth recording:
tox -e installed. It derived the repo root from the imported module, which under an sdist install resolves to site-packages, where there is no tests tree; theis_dir()guard then skipped everything and the test still passed. Reproduced (138 files swept, zero tests) and fixed by deriving the root from the test file, which is in the checkout under any import mode.docs/holds marimo notebooks excluded from both mypy and F401, the one surface no gate covered. Added to the sweep.__all__soimport *stays tidy. Not taken: exactly one of 127 modules undermloda/coredeclares__all__and it is an__init__.py, the two sibling modules from this same split declare none, and there are no star imports of anymloda.coremodule in the repo. It would reintroduce the hand-maintained list this PR removes.