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

Support for custom config classes #136

Merged
merged 2 commits into from
Sep 13, 2022
Merged

Conversation

solnic
Copy link
Member

@solnic solnic commented Aug 6, 2022

No description provided.

@solnic solnic force-pushed the support-custom-config-classes branch from 446f7d6 to 5bc4da4 Compare August 6, 2022 10:40
@solnic solnic changed the title [wip] support for custom config classes Support for custom config classes Aug 6, 2022
@solnic solnic marked this pull request as ready for review August 6, 2022 10:41
@solnic solnic requested a review from timriley August 6, 2022 10:41
@solnic solnic force-pushed the support-custom-config-classes branch from 5bc4da4 to 4c11e39 Compare August 6, 2022 10:43
@solnic
Copy link
Member Author

solnic commented Aug 21, 2022

@timriley bump :)

@timriley
Copy link
Member

@solnic This is looking good to me so far! Feels like it'd be worth following through in this PR to support custom config classes on nested settings, because without that, this single "outer" custom config class may be of fairly limited value (i.e. I know we couldn't port Hanami::Action's configuration to vanilla dry-configurable at that point; we'd need the custom nested config classes too).

@solnic
Copy link
Member Author

solnic commented Aug 25, 2022

@timriley you mean something like this:

setting :db do
  setting :host, config: MyHostConfig do
    # ...
  end
end

??

@timriley
Copy link
Member

@solnic yep, exactly this!

@solnic
Copy link
Member Author

solnic commented Aug 25, 2022

@timriley OK I'll make it possible in this PR

@solnic solnic force-pushed the support-custom-config-classes branch from 4c11e39 to 7f518de Compare September 11, 2022 15:22
@solnic
Copy link
Member Author

solnic commented Sep 11, 2022

@timriley I called the option config_class to avoid confusion with the actual config instance. Looks like things are working 🙂

@solnic solnic force-pushed the support-custom-config-classes branch from 7f518de to 442ae46 Compare September 11, 2022 15:24
@timriley
Copy link
Member

@solnic Looks great! And I agree on config_class being a clearer name.

Let's get this merged! Huge improvement for dry-configurable! 🎉

Once this is merged I'll update #138 to fit with the new structure.

@solnic solnic force-pushed the support-custom-config-classes branch from 442ae46 to 297d8c8 Compare September 13, 2022 17:39
@solnic solnic merged commit b05846f into main Sep 13, 2022
@solnic solnic deleted the support-custom-config-classes branch September 13, 2022 17:40
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.

2 participants