search

stmichael / data-import

sequel based dsl to migrate data from a legacy database to a new home.
MIT License
42 stars 9 forks source link

Partial data import #49

Open stmichael opened 12 years ago

stmichael commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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)
senny commented 12 years ago

@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.

stmichael commented 12 years ago

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

stmichael commented 11 years ago

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

stmichael commented 11 years ago

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 commented 11 years ago

What do you think about it now?

senny commented 11 years ago

@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.