Skip to content

refactor: remove resolution-epic dead code and document the new resolution surfaces - #875

Merged
TomKaltofen merged 1 commit into
mainfrom
refactor/os-015-resolution-cleanup
Jul 28, 2026
Merged

refactor: remove resolution-epic dead code and document the new resolution surfaces#875
TomKaltofen merged 1 commit into
mainfrom
refactor/os-015-resolution-cleanup

Conversation

@TKaltofen

@TKaltofen TKaltofen commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Post-epic cleanup for the FeatureGroup resolution work, in two ordered steps: remove dead code left after the epic, then document the new resolution surfaces against what remains.

Dead code removed

  • FeatureChainParser._match_pattern_based_feature: only self-referenced; the docstring claim that match_configuration_feature_chain_parser used it was stale.
  • format_feature_group_classes (plural) and its dedicated test file. The singular format_feature_group_class stays (used by the engine and covered by the error-message tests).
  • The log_n_subclasses parameter and its unreachable, buggy debug branch in get_all_subclasses.
  • Options.add, a zero-caller legacy migration shim.

Behavior-preserving tidy-up

get_column_base_feature now splits on the exported COLUMN_SEPARATOR constant instead of a bare "~", giving the constant an internal consumer. COLUMN_SEPARATOR == "~", so behavior is identical.

Diagnostic seam blessed (not retired)

_strict_validation_rejection_reason and its helper name_path_presence_rejection_reason are the assertion seam across roughly a dozen test modules. Rather than retire them, their docstrings now mark them as a supported diagnostic seam (docstring-only, no behavior change), mirroring the os-014 "bless the test seam" precedent.

Docs

  • Troubleshooting page: resolution failures are reframed around the typed FeatureResolutionError (a ValueError subclass) with a catching example.
  • mloda-api.md: diagnose documented next to explain, resolution_report() next to resolved_plan(); the stale resolve_feature block replaced with a pointer to the accurate discover-plugins.md; FeatureResolutionError, ResolutionDiagnosis, and ResolutionRecord documented.
  • One-line docstrings added to mlodaAPI.get_result() / get_artifacts().
  • memory-bank/activeContext.md and progress.md refreshed for the finished resolution epic.

Breaking changes (public API)

Two symbols on mloda's public surface are removed. Both were explicitly sanctioned for removal by the ticket and verified zero-caller across mloda core and mloda-registry:

  • Options.add(key, value) (reachable via from mloda.user import Options) is removed. Use Options.add_to_group(key, value) instead, or add_to_context(key, value) for metadata-only options.
  • get_all_subclasses(cls, log_n_subclasses=...) (exported via mloda.provider) loses its log_n_subclasses parameter. No internal call site passed it, and its only effect was an unreachable debug branch that indexed the subclass set rather than logging a count. Call get_all_subclasses(cls).

Validation

tox green (pytest -n 8, ruff format + check, pip-licenses, mypy --strict, bandit). The two public-API removals above were reviewed for backward-compatibility impact and are intentional per the ticket.

@TKaltofen
TKaltofen requested a review from TomKaltofen as a code owner July 22, 2026 10:35
@TKaltofen

Copy link
Copy Markdown
Collaborator Author

Follow-up commit: restored the 3 direct unit tests for the singular format_feature_group_class, which stays live in production (engine, execution plan, feature-group step). Deleting the plural's dedicated test file had also removed the only direct coverage of the singular formatter (the error-message tests exercise it only incidentally). New file: tests/test_core/test_abstract_plugins/test_feature_group/test_format_feature_group_class.py. tox green (7175 passed, 170 skipped).

@TKaltofen TKaltofen changed the title refactor: remove resolution-epic dead code and document the new resolution surfaces (os-015) refactor: remove resolution-epic dead code and document the new resolution surfaces Jul 22, 2026
…ution surfaces

Post-epic cleanup for the FeatureGroup resolution work, in two parts: remove dead
code left after the epic, then document the surviving resolution surface.

Dead code removed:
- FeatureChainParser._match_pattern_based_feature (only self-referenced).
- format_feature_group_classes (plural) and its dedicated test file; the singular
  format_feature_group_class stays and keeps direct unit tests in a dedicated file.
- the log_n_subclasses parameter and its unreachable debug branch in get_all_subclasses.
- Options.add, a zero-caller legacy shim.

Behavior-preserving: get_column_base_feature now consumes the exported COLUMN_SEPARATOR.
The _strict_validation_rejection_reason / name_path_presence_rejection_reason diagnostic
seam is documented as supported (docstring-only, no behavior change).

Docs: typed FeatureResolutionError framing plus a catching example on the troubleshooting
page; diagnose / resolution_report() and the FeatureResolutionError / ResolutionDiagnosis /
ResolutionRecord types in mloda-api.md; get_result / get_artifacts docstrings.

Breaking changes (public API): Options.add is removed (use add_to_group); get_all_subclasses
loses its log_n_subclasses parameter. Both were zero-caller and intentional.
@TKaltofen
TKaltofen force-pushed the refactor/os-015-resolution-cleanup branch from 8d21c0f to ff34662 Compare July 28, 2026 13:33
@TomKaltofen
TomKaltofen merged commit 2d10c2e into main Jul 28, 2026
18 checks passed
@TomKaltofen
TomKaltofen deleted the refactor/os-015-resolution-cleanup branch July 28, 2026 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants