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

push normal original args in kwargs strategy #32

Merged
merged 2 commits into from
Mar 16, 2017

Conversation

hbda
Copy link
Contributor

@hbda hbda commented Oct 18, 2016

In some cases it may be necessary to inject the classes with parameters in initializer. For example ActiveJob tasks. We improve kwargs strategy. It can work with parameterized initializers now.

@timriley
Copy link
Member

This looks like a nice enhancement! Would you mind adding a test to demonstrate/verify this extra behaviour in action?

@timriley
Copy link
Member

And I think if we wanted to put this in place, we should extend the same behaviour to the hash strategy.

@timriley
Copy link
Member

Hi @hbda, just checking in – were you interested in finishing this PR off? I left a couple of suggestions in my comments.

@hbda
Copy link
Contributor Author

hbda commented Nov 29, 2016

Hi @timriley, I plan finish this request, but I didn`t have time. I will try to finish in the near future.

@vladra
Copy link
Contributor

vladra commented Dec 1, 2016

@hbda I have this covered with tests, but without hash implementation. Take a look if you want to speed this up a little

@hbda
Copy link
Contributor Author

hbda commented Dec 11, 2016

@vladra well. I like it. Will you make request to my fork? Or I will copy your code?

@vladra
Copy link
Contributor

vladra commented Jan 29, 2017

@hbda how about triggering tests once more?

@solnic
Copy link
Member

solnic commented Jan 29, 2017

Can this be rebased on top of master?

@hbda
Copy link
Contributor Author

hbda commented Feb 3, 2017

@solnic I rebased on top of master

@hbda
Copy link
Contributor Author

hbda commented Feb 3, 2017

@timriley I with @vladra added the tests. But I have difficulties with adding the same behaviour to the hash strategy. Can you accept request without it?

@timriley
Copy link
Member

This looks good, thanks @hbda. I'll merge this now :) Would you mind letting me know what difficulty you had with the hash strategy, though?

@timriley timriley merged commit 46842b5 into dry-rb:master Mar 16, 2017
@timriley
Copy link
Member

I'll aim to make a release next week, BTW.

@flash-gordon
Copy link
Member

With a minor delay, I did this 🎉

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

Successfully merging this pull request may close these issues.

5 participants