feat: add _resolve_operation helper to FeatureChainParserMixin#242
Merged
TomKaltofen merged 5 commits intomainfrom Mar 26, 2026
Merged
feat: add _resolve_operation helper to FeatureChainParserMixin#242TomKaltofen merged 5 commits intomainfrom
TomKaltofen merged 5 commits intomainfrom
Conversation
TomKaltofen
reviewed
Mar 24, 2026
mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser_mixin.py
Show resolved
Hide resolved
TomKaltofen
reviewed
Mar 24, 2026
| def _resolve_operation( | ||
| cls, | ||
| feature_name: Any, | ||
| options: Options, |
Collaborator
There was a problem hiding this comment.
- update docstring
- update /docs
- create an issue if the docs guides should be adjusted. Only if worth it
TomKaltofen
reviewed
Mar 25, 2026
|
|
||
| def test_feature_shorthand_config_fallback(self) -> None: | ||
| """Feature shorthand falls back to options when pattern does not match.""" | ||
| from mloda.user import Feature |
Collaborator
There was a problem hiding this comment.
move all the mloda.user import Feature to top level import
TomKaltofen
reviewed
Mar 25, 2026
tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_resolve_operation.py
Show resolved
Hide resolved
TomKaltofen
reviewed
Mar 25, 2026
|
|
||
| def test_works_with_feature_name_object(self) -> None: | ||
| """Accepts FeatureName objects as well as strings.""" | ||
| from mloda.core.abstract_plugins.components.feature_name import FeatureName |
Collaborator
There was a problem hiding this comment.
move this to toplevel import
TomKaltofen
reviewed
Mar 25, 2026
- Move imports to top level in test file (FeatureName, Feature) - Add mloda.run_all() integration tests (string-based and config-based) - Improve _resolve_operation docstring with Args section - Create mloda-registry issue #58 for docs guide updates - Ruff formatting fixes
…gFeatureGroup Replace manual dual-path parsing (FeatureChainParser.parse_feature_name + options fallback) with the new _resolve_operation helper in: - AggregatedFeatureGroup._extract_aggregation_type - ScalingFeatureGroup._extract_scaler_type
TomKaltofen
reviewed
Mar 26, 2026
Collaborator
There was a problem hiding this comment.
Do we need this function? Refactor it.
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
2 participants
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.
Summary
_resolve_operation(cls, feature_name, options, config_key)classmethod toFeatureChainParserMixinoptions.get(config_key)FeatureChainParser.parse_feature_namedirectly, reducing boilerplate in data-ops feature groupsCloses #237
Test plan
ruff checkandmypy --strictclean