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

Replace dep on dry-configurable with a simple Configuration mod #84

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

solnic
Copy link
Member

@solnic solnic commented Jul 1, 2022

This removes dependency on dry-configurable as dry-container is a lower-level gem and it shouldn't depend on dry-configurable, which is a higher-level gem. Originally, this wasn't the case, but dry-configurable has grown into a quite advanced configuration system (and will most likely continue to evolve).

Configuration needs of dry-container are very basic, they don't justify the usage of dry-configurable. Notice that dry-container is one of the oldest dry-rb gems and throughout the years its config (3 config values: namespace separator, registry and resolver) have been always the same.

Stuff works the same now:

irb(main):001:1* class MyContainer
irb(main):002:1*   include Dry::Container::Mixin
irb(main):003:0> end
=> MyContainer
irb(main):004:0> MyContainer.config.namespace_separator = "/"
=> "/"
irb(main):005:0> mc = MyContainer.new
=> #<MyContainer:0x00007f9a68962a30 @_container={}>
irb(main):006:0> mc.namespace("foo") { |foo| foo.register("bar", :bar) }
=> #<MyContainer:0x00007f9a68962a30 @_container={"foo/bar"=>#<Dry::Container::Item::Callable:0x00007f9a689df990 @item=:bar, @options={:call=>false}>}>
irb(main):007:0> mc["foo/bar"]
=> :bar

Copy link
Member

@flash-gordon flash-gordon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, dry-configurable's features propagate to dry-system, and someone may use them. Maybe dry-system's container should continue to have them. Other than this, 👍

@solnic
Copy link
Member Author

solnic commented Jul 5, 2022

Currently, dry-configurable's features propagate to dry-system, and someone may use them. Maybe dry-system's container should continue to have them. Other than this, 👍

Yeah so dry-system's extends its container with Dry::Configurable and the gem depends on dry-configurable explicitly. So this is not a problem (luckily 😅).

@solnic
Copy link
Member Author

solnic commented Jul 7, 2022

@timriley are you OK with this?

@timriley
Copy link
Member

timriley commented Jul 7, 2022

@solnic Don't forget to remove dry-configurable from project.yml! Otherwise the change in the gemspec will get overridden.

@timriley
Copy link
Member

timriley commented Jul 7, 2022

Thanks for making this change, @solnic, glad to see it go in!

@solnic solnic force-pushed the remove-configurable-dep branch from b50f34b to 854ad21 Compare July 8, 2022 08:41
@solnic solnic merged commit 659cd2c into main Jul 8, 2022
@solnic solnic deleted the remove-configurable-dep branch July 8, 2022 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants