data-import icon indicating copy to clipboard operation
data-import copied to clipboard

Partial data import

Open stmichael opened this issue 12 years ago • 8 comments

I started by decoupling the lookup tables from the definitions. It's not finished yet. There is still code that uses lookups on definitions.

@senny what do you think about it so far?

stmichael avatar Sep 27 '12 06:09 stmichael

Also references to lookup tables are still hold on the definitions. If we really want to make the lookup an independent feature, we have to put it somewhere else.

stmichael avatar Sep 27 '12 06:09 stmichael

I created an id mapping container which holds all id dictionaries. The container is held by the execution plan. I'm not happy the way it is used because I have to pass it to several initializations. @senny any idea on how to change that? May the container is in the wrong place.

The syntax to use the id mappings in the blocks is now:

id_mapping_for('definition name', 'id lookup name').lookup(old_value)
id_mapping_for('definition name', 'id lookup name').add(old_value, new_value)

stmichael avatar Sep 27 '12 14:09 stmichael

@stmichael I think the IdMappingContainer belongs to the ExecutionPlan. I think the DSL objects should always have access to the plan, which is being built. They are coupled to the plan very tightly anyways because their only intention is to build the plan. So I suggest you pass along the whole plan in place of only the mapping container. I also think you should bridge the methods to add a new dictionary on the ExecutionPlan so that you don't need to reach through the object to grab the container.

senny avatar Sep 27 '12 14:09 senny

I updated the branch with the discussed changes. @senny If you agree I will go on to the implementation of the partial import

stmichael avatar Sep 27 '12 14:09 stmichael

This is a first version of a partial import. What do you think about it?

stmichael avatar Oct 29 '12 08:10 stmichael

The next step would be to extract the loading and storage of the settings into a separate class to keep it together. Also in the unit test of the partial import I directly write to the settings file as test setup. That's not flexible since it's closely tied to the implementation.

stmichael avatar Oct 29 '12 08:10 stmichael

What do you think about it now?

stmichael avatar Oct 30 '12 07:10 stmichael

@stmichael I'm a little short on review time at the moment. I have some general spots I'm not that happy with but don't have prepared suggestions how to improve:

  1. The Naming seems a bit off. (eg. SettingsStore does not store settings but more progress related data, also the IdMappingsXXXX part does not communicate clearly to me. => very subjective comment)
  2. I would not use Inheritance in the Migration Strategy but Composition.
  3. I'm not sure if we want a two step process to lookup an ID, eg: id_mapping_for(...).lookup(...). This was the source that we had to deprecate the complete API and I don't think we should do that in the future.

senny avatar Nov 01 '12 08:11 senny