Breakup IModelTransformer class#295
Conversation
| targetScopeElementId: Id64String; | ||
| isProvenanceInitTransform?: boolean; | ||
| allowNoScopingESA?: boolean; | ||
| hasArgsForProcessChanges?: boolean; |
There was a problem hiding this comment.
- Who
hasArgsForProcessChanges? Name makes no sense inSyncTypeResolverclass. - Why all these values are put into options? Why those are not passed separately into constructor?
- Why
determineSyncType()is static and public?
It seems that there are some rules how to use this class, but it is not intuitive to grasp from code / implementation. I see that getResolvedSyncType() throws if resolve() was not called before, but when is the right time to call resolve? Maybe better implementation would help to lift that requirement.
There was a problem hiding this comment.
I cleaned up this a bit. Removed options as I agree, they weren't necessary (I actually thought I removed them at first but what copilot actually did was remove move the options interface it created from imodelTransformer to the new files)
determineSyncType was always static and public I believe, so I kept it the same. Do you think it should change to private now that I moved it into its own class? I believe this design decision was originally made so users could potentially do this:
/** Create an ExternalSourceAspectProps in a standard way for an Element in an iModel --> iModel transformation. */
public static initElementProvenanceOptions(
sourceElementId: Id64String,
targetElementId: Id64String,
args: {
sourceDb: IModelDb;
targetDb: IModelDb;
// TODO: Consider making it optional and determining it through ESAs if not provided. This gives opportunity for people to determine it themselves using public static determineSyncType function.
isReverseSynchronization: boolean;
targetScopeElementId: Id64String;
}
): ExternalSourceAspectProps {
return ProvenanceManager.initElementProvenanceOptions(
sourceElementId,
targetElementId,
args
);
}| /** Options for constructing a ProvenanceManager. | ||
| * @internal | ||
| */ | ||
| export interface ProvenanceManagerOptions { |
There was a problem hiding this comment.
- Why all these properties are put under
ProvenanceManagerOptionsinstead of passing them separately into ctor? - Why
ProvenanceManagerOptionshastransformerOptions, does it help to decouple implementation? - Design and usage of
getIsReverseSynchronization()/queryTargetRelationshipId()feels over-complicated:
There was a problem hiding this comment.
Design and usage of getIsReverseSynchronization() / queryTargetRelationshipId() feels over-complicated
I didn't like this either, but these 2 functions depend on imodelTransformer context that the provenance manager doesn't have. I feel like queryTargerRelId could be moved to manager, but that function depends on the transformer context, which I don't think we want to pass to the manager.
Similar with getIsReverseSync, we would need to pass in the sync type resolver to the provenance manager
| (this.targetDb as any).codeValueBehavior = "exact"; | ||
| } | ||
| /* eslint-enable @itwin/no-internal */ | ||
| this._syncTypeResolver = new SyncTypeResolver({ |
There was a problem hiding this comment.
What is the plan for unit tests? Will it be easy to test / mock when values are not passed trough ctor?
There was a problem hiding this comment.
my new impl should make make testing/mocking easy, but lmk if that's not what you were referring to
This was a suggestion by copilot and I think aligned a little with one of the comments in the service's teams issue about reducing size of some of the files. I reviewed this work as copilot was making the changes, and I feel its fine. However, I would like to get the perspective of the service team. These are pretty dramatic changes, and I don't necessarily know if I agree that its worthwhile.
I had Copilot do a write up for what the documentation on this would look like
@itwin/imodel-transformer — Internal Architecture: Decompose IModelTransformer
Summary
The
IModelTransformerclass has been decomposed from a ~3,500-line monolith into a focused orchestrator (~2,500 lines) backed by three new internal classes. This is a non-breaking change — the public API surface is unchanged.What Changed
Internal Class Extractions
Three new internal (non-exported) classes were extracted from
IModelTransformer:Removed Items (Dead Code)
If you referenced any of the following (unlikely — they were undocumented internals), they no longer exist:
_lastProvenanceEntityInfomarkLastProvenance()nullLastProvenanceEntityInfoLastProvenanceEntityInfoDesign Decisions
IModelTransformerand forward to the extracted class. This keeps the public surface stable.ProvenanceManagerreceives capabilities likegetIsReverseSynchronizationandqueryTargetRelationshipIdas constructor callbacks, avoiding circular dependencies between extracted classes.ProvenanceManagerholds a reference to the transformer's_optionsobject (not a copy) so that post-construction mutations (common in tests) remain visible.ChangesetPlannerin a future iteration.