-
Notifications
You must be signed in to change notification settings - Fork 9
Provide options to import mapping execution contexts. #54
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
base: master
Are you sure you want to change the base?
Conversation
Light on changes to unit tests; new acceptance spec added.
I don't think helper modules would cover my use case. I don't think this is related really - helper modules are static extensions to the execution context, whereas I needed to make them configurable. That said, I would find helper modules useful, hence my comments on the original ticket. |
@andyt why not use ENV variables for this configuration? |
Your solution is very specific to your use case. I don't like the idea of passing the options all around the code just to make them available in the execution context. |
Extending the context of a mapping would seem to be quite a common requirement. You might want to do this for configuration or scoping, perhaps based on some selections in an interface. However, I agree that passing options everywhere isn't elegant, although that's a side effect of the multiple contexts used. If there was one context (if such a thing was possible), it wouldn't be so pervasive. In my use case, ENV variables won't work. I'm using your library within https://github.com/mperham/sidekiq, which is multi-threaded. I would prefer to avoid using Thread.current, which would be another option. Anyway, I'm open to other suggestions. Thanks again for a great library. |
@andyt Did you find any solution for your problem? I just had another idea. Suppose these helper modules from issue #38 already existed. How about you take the parameters you receive from sidekiq and feed them into a module which will then be included in the import blocks. That's more structured than just defining global variables und the DSL won't get that much complicated. |
I think your suggestion suffers from the same problem - unless I can make the module name for inclusion dynamic, then surely I can only include the same module each time, and I have to resort to using Thread.current. Or am I misunderstanding your suggestion? |
I don't know your exact use case and the environment in which you'd like to use data-import. I assumed you wanted to pass some arguments you received from sidekiqs
And in the mapping you include @andyt what do you think? |
Yes, that's the kind of thing I thought you were suggesting. Wouldn't all threads in a process access the same options? |
Oh, now I see your point. Sorry, it took so long 😢 I don't know I'm not a great fan of background job libraries that run in one process like Of course it would have been wise to make the implementation more memory aware. But still, I find it odd that such jobs make the machine freeze. I mean after a job ran I would expect it to vanish completely and not leaving any traces (except the ones it SHOULD leave). And that's why I love Would that be an option for you? |
I like Resque too, but Sidekiq has a better memory footprint (given no leaks!). We could switch to Resque, but the commit I made in my fork is working for me. |
You might want a few more unit tests, and there's a possibility this doesn't cover all execution contexts - though it meets all mine.
I've added a new acceptance spec.