search

dry-rb / dry-container

A simple, configurable object container implemented in Ruby
https://dry-rb.org/gems/dry-container
MIT License
335 stars 41 forks source link

resolve items relative to namespace #47

Closed yuszuv closed 6 years ago

yuszuv commented 6 years ago

With this pull request you can design your container namespaces without knowing anything about the parent namespace (like names, etc.)

timriley commented 6 years ago

This looks good to me. What do you think, @AMHOL?

What I'd actually love is for this to go one step further and for Dry::Container::Mixin#namespace to actually return a namespace-scoped container object that can be passed around for when only a subset of the container is required (right now it only allows working with the namespaced container via the block, and returns self).

AMHOL commented 6 years ago

Seems fair to me, the only thing to consider is the possibility of use-cases where people are resolving from higher-level namespaces already, which will no longer be possible without a local variable or something.

RE: returning a scoped container, sounds like a cool idea to me, with the current implementation we wouldn't actually be able to reduce the items in the container that is passed around, as they are stored as a flat hash, but we could restrict the namespace pre-resolution.

yuszuv commented 6 years ago

@AMHOL I don't think resolving higher-level namespace items is a problem, since resolving items via namespace#resolve isn't documented anywhere and self#resolve does work as before, resolving items relative to the top level. So the worst thing is, that people using the undocumented namespace#resolve have to remove the namespace prefix from that method's arguments.

AMHOL commented 6 years ago

Ahh yes, I missed that, thanks for clearing it up. :smile:

solnic commented 6 years ago

So the worst thing is, that people using the undocumented namespace#resolve have to remove the namespace prefix from that method's arguments.

Please triple-check, because a) dry-system may rely on original behavior b) people may rely on original behavior. If this breaks dry-system and/or dry-web apps, we need 0.x bump and clear info in changelogs.

timriley commented 6 years ago

I will check dry-system (and our apps) with this next week.

On 14 Jun 2018, at 9:02 pm, Piotr Solnica notifications@github.com wrote:

So the worst thing is, that people using the undocumented namespace#resolve have to remove the namespace prefix from that method's arguments.

Please triple-check, because a) dry-system may rely on original behavior b) people may rely on original behavior. If this breaks dry-system and/or dry-web apps, we need 0.x bump and clear info in changelogs.

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