iTwin / imodel-transformer

API for exporting an iModel's parts and also importing them into another iModel
MIT License
3 stars 2 forks source link

Added additional args to Exporter.exportChanges #35

Closed simsta6 closed 1 year ago

simsta6 commented 1 year ago
MichaelBelousov commented 1 year ago

what is the purpose of this change? What is a use case for setting it?

simsta6 commented 1 year ago

what is the purpose of this change? What is a use case for setting it?

In our service, we need ChangedInstanceIds data. To reuse this data, I decided to add a public getter because adding a hook for this is not worth it.

I see that this change broke one test. In failing test the same transformer instance is used and it's expected that ChangedInstanceIds.initialize will get called.

Should I add a hook to IModelExporter or do something else?

MichaelBelousov commented 1 year ago

In our service, we need ChangedInstanceIds data. To reuse this data, I decided to add a public getter because adding a hook for this is not worth it.

Maybe you can make it an option like sourceChangeData? I'd rather not expose a setter that can be used at any time.

MichaelBelousov commented 1 year ago

I see that this change broke one test. In failing test the same transformer instance is used and it's expected that ChangedInstanceIds.initialize will get called.

Should I add a hook to IModelExporter or do something else?

I will review after you do it, but you can just add a comment explaining that it may not get called if already set, and remove the check. But I'm not sure why your changes would prevent it from being called in the default case (although I haven't read the code recently).

simsta6 commented 1 year ago

Note that I generated minor version change