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

resolve items relative to namespace #47

Merged
merged 1 commit into from
Jun 14, 2018

Conversation

yuszuv
Copy link

@yuszuv yuszuv commented Jun 10, 2018

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

@timriley
Copy link
Member

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
Copy link
Member

AMHOL commented Jun 13, 2018

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
Copy link
Author

yuszuv commented Jun 13, 2018

@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
Copy link
Member

AMHOL commented Jun 14, 2018

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

@AMHOL AMHOL merged commit 8f99bc6 into dry-rb:master Jun 14, 2018
@solnic
Copy link
Member

solnic commented Jun 14, 2018

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
Copy link
Member

timriley commented Jun 14, 2018 via email

niels-s pushed a commit to blendle-forks/dry-container that referenced this pull request Feb 20, 2019
PR dry-rb#47 introduced feature to resolve relative to the namespace but there
wasn't an option to resolve keys from other namespaces or the root.
This changes gives people the option to disable the relative
namespacing
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.

4 participants