Add convenience method Dry::Configurable::Settings#replace

[skinnyjames]

Hiya,
Really new to this project and dry-rb in general, so I don't expect to hit the mark on my first try, but this PR fixes #109

I'm curious if this is the desired api, or if it would be easier to copy settings from another Dry::Configurable class or object without accessing what I am assuming to be a an internal var?

I was thinking..

  class One
    extend Dry::Configurable
    setting :hello, "world"
  end

  class Two
    extend Dry::Configurable
    setting :hello, "bazz"
  end

  Two.clobber_config(One)
  
  One.config.hello #=> "World"
  Two.config.hello #=> "World"

[solnic]

Thanks for working on this. It would be great to expose Settings object via #settings method, rather than this set object which is just some legacy we have (@timriley do you remember why it's even there?). Then you could just do things like ChildClass.settings.replace(ParentClass.settings) etc.

I'm working on integrating dry-configurable into rom-rb's core at the moment and methods like replace or merge or append etc. would be incredibly useful. I actually think some "settings and config algebra" should be a 1st-class feature in this library and it would make it really stand out. There are a lot of interesting usage patterns that involve merging/inheriting configuration that can actually be really tricky to deal with manually.

[skinnyjames]

No problem!
I didn't realize that I was still failing checks. I'll get that cleaned up tonight.
I can also work on merge / append too :)

dumb question: can I run rubocop locally to run the linter?

[flash-gordon]

@skinnyjames sure, bundle exec rubocop should do

[skinnyjames]

Fixed up the linting hopefully!
I also added Settings#merge functionality in my origin branch
https://github.com/skinnyjames-forks/dry-configurable/pull/1/files

If that seems like a good add, I'm happy to add it to the PR.

[timriley]

It would be great to expose Settings object via #settings method, rather than this set object which is just some legacy we have (@timriley do you remember why it's even there?). Then you could just do things like ChildClass.settings.replace(ParentClass.settings) etc.

@solnic I don't have any clue why it's the way it is, unfortunately! I presume we've simply been preserving some very early API decision without giving it a whole ton of thought.

I agree that the set object would be better replaced by the full Settings instance — let's consider that for a follow up PR? Be good to do that pre-1.0.

I'll leave some other thoughts on this PR itself in another comment shortly 😄

[timriley]

Thanks for getting to work on this, @skinnyjames! Really appreciate your effort!

Now, I hope you don't mind, but I've left a few somewhat extensive comments on the code here. We're definitely heading in the right direction, but there are some details we'll want to get right and extra scenarios we should consider before this is a fully resolved new feature.

Anyway, please have a read and let me know if you think anything needs clarifying. Am definitely happy to work with you (and @solnic) as we iterate on this feature!

[timriley]

BTW, @skinnyjames, regarding your original comments in the PR description:

I'm curious if this is the desired api, or if it would be easier to copy settings from another Dry::Configurable class or object without accessing what I am assuming to be a an internal var?

I was thinking..

  class One
    extend Dry::Configurable
    setting :hello, "world"
  end

  class Two
    extend Dry::Configurable
    setting :hello, "bazz"
  end

  Two.clobber_config(One)
  
  One.config.hello #=> "World"
  Two.config.hello #=> "World"

I think the approach you chose in the actual code changes you've made is the right one. 👍🏼

Since Dry::Configurable can be mixed into all sorts of classes, I don't think we want to add too many extra methods to those classes. By sticking to setting defining new settings, and then settings (_settings right now, but I think we can rename it, per my comments to @solnic earlier), we can then allow most of the extra behaviour to hang off that settings object rather than polluting the method list of whatever class is mixing in our module.

[skinnyjames]

I'm noticing a weird state issue when invoking the config accessor before merging the settings.
this passes.

        klass.setting :database do
          setting :dsn, "localhost"
        end
        other_klass.setting :database do
          setting :dsn, "remote"
        end

        other_klass._settings.merge!(klass._settings)
        expect(other_klass.config.database.dsn).to eql("localhost")

but this fails.

        klass.setting :database do
          setting :dsn, "localhost"
        end
        other_klass.setting :database do
          setting :dsn, "remote"
        end
        expect(other_klass.config.database.dsn).to eql("remote")
        other_klass._settings.merge!(klass._settings)
        expect(other_klass.config.database.dsn).to eql("localhost")

For now, I'm not at all sure why that is happening.
adding failing tests in the meantime.

[timriley]

Thanks @skinnyjames — just left you a couple more notes in the previous batch of comments. I'll take a look at the failing specs in the next day or two, I hope.

[skinnyjames]

Thanks @skinnyjames — just left you a couple more notes in the previous batch of comments. I'll take a look at the failing specs in the next day or two, I hope.

No problem. The api should be a little more clear now that "merge" returns settings, and "merge!" will merge those settings. I think the reason the tests will fail is because the config is cached once invoked. It seems intentional though.