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

Retain access to injector’s container when defining .new method #31

Merged
merged 5 commits into from
Oct 7, 2016

Conversation

timriley
Copy link
Member

@timriley timriley commented Oct 7, 2016

This allows multiple injectors in various superclasses, using different containers, to continue to provide their expected dependencies when an instance of a subclass is initialized.

Resolves #30, enabling this to work as you'd expect:

container_one = {"one" => "hi from one"}
container_two = {"two" => "hi from two"}

InjectOne = Dry::AutoInject(container_one).kwargs
InjectTwo = Dry::AutoInject(container_two).kwargs

class One
  include InjectOne["one"]
end

class Two < One
  include InjectTwo["two"]
end

Two.new
# => #<Two:0x007fa667cfd950 @one="hi from one", @two="hi from two">

Previously, @one was nil because the reference to InjectOne's generated .new method container was just looking to whatever container was associated with the last mixed-in injector (i.e. container_two in this example).

@flash-gordon – I'm touching on some of the stuff that you tidied up last, does this seem OK to you?

This allows multiple injectors in various superclasses, using different containers, to continue to provide their expected dependencies when an instance of a subclass is initialized.
@flash-gordon
Copy link
Member

@timriley sure, no worries. Any reason why you left the args strategy untouched? If you update it in a similar way, we wouldn't need that ambiguous container accessor.

@timriley
Copy link
Member Author

timriley commented Oct 7, 2016

I left args untouched mostly because it's not sensible to use it with multiple injectors like my example, but you're right, it'd be a great chance to clean things up! I'll do that soon :)

On 7 Oct. 2016, at 10:20 pm, Nikita Shilnikov notifications@github.com wrote:

@timriley sure, no worries. Any reason why you left the args strategy untouched? If you update it in a similar way, we wouldn't need that ambiguous container accessor.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@timriley
Copy link
Member Author

timriley commented Oct 7, 2016

Made that tidy-up, @flash-gordon, thanks for the pointer! I'll merge this in now.

@timriley timriley merged commit 3f92161 into master Oct 7, 2016
@flash-gordon flash-gordon deleted the retain-access-to-container-in-superclasses branch May 25, 2017 20:11
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.

2 participants