-
-
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
Forward keyword args to initialize #113
Conversation
df011af
to
482402e
Compare
if RUBY_VERSION >= "2.7" | ||
def initialize(*, **) | ||
@config = Config.new(self.class._settings.dup) | ||
super | ||
end | ||
else | ||
def initialize(*) | ||
@config = Config.new(self.class._settings.dup) | ||
super | ||
end | ||
end |
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.
To keep compatibility with 2.6, I tried various incantations of this method using ruby2_keywords
(as suggested on https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/), but I couldn't get any to work.
Given this is a simple method, and given we're likely to drop support 2.6 fairly soon, and that we're not having to do this anywhere else within the lib, I'm OK with the duplication here.
Maybe you might have some ideas about how to handle this better, @flash-gordon?
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.
doesn't this work?
module Initializer
# @api private
def initialize(*)
@config = Config.new(self.class._settings.dup)
super
end
ruby2_keywords(:initialize) if respond_to?(:ruby2_keywords, true)
end
I tested locally on 3.0, it passed
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.
@flash-gordon it didn't work with the older versions, IIRC
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.
@flash-gordon By gum, you're right! I think I was trying it with *args, **kwargs
and that was messing it up.
@solnic @flash-gordon lemme know if you're happy for this to go in :) I'd like to do a new patch release with this included so I can depend on it from hanami. |
def initialize(*) | ||
@config = Config.new(self.class._settings.dup) | ||
super | ||
end |
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.
I think we can levarage ...
in >= 3.0 too, so:
if RUBY_VERSION >= "3.0"
def initialize(...)
@config = Config.new(self.class._settings.dup)
super
end
end
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.
- it's 2.7
- it'll require using strings and class_eval since it's a syntax change. I'd postpone
...
till 2.6 is EOL
This fixes an ArgumentError that would have been raised for classes including `Dry::Configurable` whose `#initialize` has required keyword parameters
482402e
to
a6839d1
Compare
@solnic @flash-gordon Pushed another commit showing the simplified version working. If you're good with this I'll do a squash when I merge the PR. |
This fixes an ArgumentError that would have been raised for classes including
Dry::Configurable
whose#initialize
has required keyword parameters.This is required to convert the base
Hanami::Configuration
class to dry-configurable, since that class has the following#initialize
(whose params signature I'd like to retain):