-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
Allow multiple includes of Dry::Configurable #164
Conversation
Instead of raising AlreadyIncludedError (now removed), update the method undef code in `.included` to handle already-undefined methods gracefully. Allowing Dry::Configurable to be included multiple times across subclasses now allows for uses cases like a subclass adjusting various extension settings, e.g. `config_class`.
@@ -36,8 +34,8 @@ def included(klass) | |||
prepend(Initializer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that these lines are reachable, things are more tricky. include
and prepend
work differently, include
will add a module to the ancestors' chain only once, whereas prepend
does it every time. We should keep this in mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I'll look to expand the testing to cover a few more possible edge cases before I mark this ready for a final review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in Expand tests
@flash-gordon FYI, this is the reason for this PR: hanami/hanami@
How does that feel to you? I thought about adding a dedicated API to |
e277605
to
c566b08
Compare
@flash-gordon I've expanded the tests to make sure nothing surprising happens with the multiple prepended Given this, are you happy to see this be merged? Thanks again for checking! |
Instead of raising AlreadyIncludedError (now removed), update the method undef code in
.included
to handle already-undefined methods gracefully.Allowing Dry::Configurable to be included multiple times across subclasses now allows for uses cases like a subclass adjusting various extension settings, e.g.
config_class
.