Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor lookup tables #48

Open
senny opened this issue Sep 25, 2012 · 2 comments
Open

refactor lookup tables #48

senny opened this issue Sep 25, 2012 · 2 comments

Comments

@senny
Copy link
Collaborator

senny commented Sep 25, 2012

Now both definitions (simple and script) need the lookup tables. We need to find a better internal solution.

@stmichael
Copy link
Owner

I'm finally continuing the development. I took a look at the branch with the partial import #49 and I'm not happy with the current state. The library need some refactoring to make partial import possible. Also the size of the branch is already too big to keep an overview. I decided to make small step and to begin with this issue here.

The lookup tables are currently too tightly coupled with the definitions. I'd like to make this a separate concept. Essentially the lookup table is a set of dictionaries. I like the name dictionary in our context, because we want to lookup an value and get its meaning.

Also the DSL for the id lookup is not that great. definition(...).lookup_for(...), definition(...).row_imported(...) and definition(...).identify_by(...) is 💩

I thought of two solutions to make the DSL more meaningful:

  • Since the lookup is tied to the definition anymore, the definition(...) part doesn't make sense anymore. Instead the execution context provides something like lookup(...) or id_dictionary which returns a dictionary, which then provides methods like find, add.
  • Or we could make the methods to update the dictionaries directly available in the DSL like find_mapped_id and add_mapped_id.

I prefer the first solution. If we provide a method for each use case in the execution context, its API will eventually explode. Also I find it difficult to find meaningful method names for the second solution.

@senny what do you think about it?

@senny
Copy link
Collaborator Author

senny commented Nov 5, 2013

@stmichael I currently have limited time and can't dig back into the source of this lib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants