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

Delegate all NestedConfiguration to its own class to fix reset_config #28

Merged
merged 2 commits into from
Feb 21, 2017

Conversation

GustavoCaso
Copy link
Member

Regarding the commit fd8ccfe where it exposes that reset_config was not working for nested configurations.

@AMHOL I have followed what we talked yesterday, and even it since a little by hacky as well, all the logic for nested configurations is encapsulated in its own class.

The PR still in WIP because I wanted to know if you like this approach if so I will add a test for everything that is new and remove the WIP label so we can merge it, if not I will keep looking for an alternative.

@AMHOL thanks for the help.

Copy link
Member

@AMHOL AMHOL left a comment

Choose a reason for hiding this comment

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

Couple of questions but looks good, nice one @GustavoCaso 👍

@klass.config
end

def method_missing(method, *args, &block)
Copy link
Member

Choose a reason for hiding this comment

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

Never seen this defined as a private method before, any implications?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I'm honest no idea, but I can move outside of private

Copy link
Member

Choose a reason for hiding this comment

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

It's fine as-is, I guess method missing will only be called privately anyway, just never thought of it before 😄

@@ -114,14 +115,30 @@ def _settings
def _config_for(&block)
config_klass = ::Class.new { extend ::Dry::Configurable }
config_klass.instance_eval(&block)
config_klass.config
create_nested_config(config_klass)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the above two lines can be moved to the new NestedConfig class too, an this could just become:

::Dry::Configurable::NestedConfig.new(&block)

Copy link
Member Author

Choose a reason for hiding this comment

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

But we still have to pass the config_klassinstance to ::Dry::Configurable::NestedConfig.

Copy link
Member

Choose a reason for hiding this comment

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

Could just update ::Dry::Configurable::NestedConfig.new to:

module Dry
  module Configurable
    # @private
    class NestedConfig
      def initialize(&block)
        klass = ::Class.new { extend ::Dry::Configurable }
        klass.instance_eval(&block)
        @klass = klass
      end
      # ...

Then it fully encapsulates the creation of nested config

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome thats great I will update the code, and the missing test so we can move forward

@GustavoCaso GustavoCaso changed the title [WIP] Delegate all NestedConfiguration to its own class to fix reset_config Delegate all NestedConfiguration to its own class to fix reset_config Feb 21, 2017
@GustavoCaso
Copy link
Member Author

@AMHOL, since is a NestedConfig doesn't have much to test in my opinion, and there is the complete integration test, for the configurable Module we could ship this.

What do you think?

@AMHOL
Copy link
Member

AMHOL commented Feb 21, 2017

@GustavoCaso seems fair to me, thanks for implementing this 👍

@AMHOL AMHOL merged commit 7a1c358 into master Feb 21, 2017
@AMHOL AMHOL deleted the fix_reset_config_for_tests branch February 21, 2017 23:06
@AMHOL
Copy link
Member

AMHOL commented Feb 21, 2017

@GustavoCaso this is now released under 0.6.1

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.

None yet

2 participants