Extract the pivot and unit-conversion blocks out of object_transformer - #331
Merged
Merged
Conversation
Both were self-contained subsystems reached from three call sites and calling nothing else in the class. object_transformer.py drops from 1558 to 1246 lines. The pivot group (MELT/UNMELT, 5 functions) moves to transformer/pivot.py. Only two of them touched instance state, and only target_schemaview, which they now take explicitly. _perform_unit_conversion moves to functions/unit_conversion.py, next to convert_units and UnitSystem, which it already depended on. It referenced no instance state at all — it took everything through DerivationContext — so it takes the three fields it used (source row, schemaview, source type) as parameters instead. Depending on DerivationContext would have been circular, since object_transformer imports this module. No behaviour change: 1102 passed, 4 skipped, 1 xfailed, unchanged. Closes #304
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restructures ObjectTransformer by extracting the self-contained pivot (MELT/UNMELT) and unit-conversion logic into dedicated modules, reducing object_transformer.py size and keeping related functionality colocated with its natural neighbors.
Changes:
- Moved pivot/EAV reshaping functions into
src/linkml_map/transformer/pivot.pyand updated call sites to passtarget_schemaviewexplicitly. - Moved unit-conversion execution logic into
src/linkml_map/functions/unit_conversion.pyasperform_unit_conversion, avoidingDerivationContextto prevent circular imports. - Updated
ObjectTransformerto call the new module-level functions and removed the in-class implementations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/linkml_map/transformer/pivot.py | Introduces module-level pivot helpers (perform_pivot_operation, perform_melt, and unmelt helpers) extracted from ObjectTransformer. |
| src/linkml_map/transformer/object_transformer.py | Rewires pivot and unit-conversion call sites to the new module functions and removes the extracted method blocks. |
| src/linkml_map/functions/unit_conversion.py | Adds perform_unit_conversion alongside existing unit utilities so conversion execution lives with convert_units/UnitSystem. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The extracted pivot functions defaulted target_sv to None, so a caller that forgot it would silently melt every non-ID slot instead of inferring from unmelt_to_class. Make it required. class_deriv, sv and source_type were never read by the unmelt path, and perform_melt ignored slot_derivation -- which is why perform_pivot_operation could pass a ClassDerivation into it without anything noticing.
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.
Closes #304. Pure move — no new abstraction, no behaviour change.
object_transformer.pydrops from 1558 to 1246 lines.Pivot / EAV →
transformer/pivot.pyFive functions (
perform_pivot_operation,_perform_unmelt,_unmelt_single_record,_unmelt_collection,perform_melt), reached from two call sites and calling nothing else in the class.An AST pass over the block confirmed the issue's claim about instance state, and it was cleaner than described — only
_unmelt_single_recordandperform_melttouchedselfat all, both solely fortarget_schemaview, which is now an explicit parameter threaded through the two callers.Unit conversion →
functions/unit_conversion.pyperform_unit_conversionlands next toconvert_unitsandUnitSystem, which it already depended on.It referenced zero instance state — everything came through
DerivationContext. But that type is defined inobject_transformer, which imports this module, so keeping the parameter would have made the import circular. It now takes the three fields it actually used (source_obj,sv,source_type) directly, which is what the issue proposed by "taking the schemaview explicitly". Verified there's no cycle by importing both modules standalone.On the #298 overlap
The issue flagged that the mock tests drove the private
_perform_unit_conversiondirectly, so whichever landed second had to account for the other. #323 landed first and replaced them with end-to-end tests throughmap_object— which is why this extraction needed no test changes at all. Had the mocks still been in place, all seven would have broken on a refactor that changes no behaviour. Confirmed nothing outside the class referenced any of the six moved methods.Verification
1102 passed, 4 skipped, 1 xfailed — identical to main, since the existing pivot (9) and unit-conversion (7) tests already exercise this code end-to-end. ruff and format clean.