-
Notifications
You must be signed in to change notification settings - Fork 186
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
Implement no args Struct.new constructor in Ruby 3.3 #3491
Conversation
@@ -33,7 +33,7 @@ class << self | |||
alias_method :subclass_new, :new | |||
end | |||
|
|||
def self.new(klass_name, *attrs, keyword_init: nil, &block) | |||
def self.new(klass_name = nil, *attrs, keyword_init: nil, &block) |
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 doesn't seem a correct change. And adding more specs would catch the issue.
Optional parameter will always consume the first argument and will be nil
only if there are no positional arguments at all. But it isn't how it's supposed to work:
Struct.new(:foo, :bar).name # => nil
Handling the first argument (if any) depends on its type. Non-Symbol argument will be handled as a class name. A Symbol argument - as a member name.
So it's more like:
if klass_name.is_a?(Symbol)
attrs.unshift(klass_name)
klass_name = nil
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.
Thanks. We'll add more specs and fix this.
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.
@andrykonchin I took another look at this and I'm a little confused. The code appears to already do what you suggest with regards to checking if the first argument is a Symbol
and I couldn't see a behavioral difference in the Struct.new(:foo, :bar).name
example. Can you please elaborate on what the problem is?
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'm not sure exactly what Andrii meant but the current logic + optional argument makes it really hard to reason about.
Optional + rest arguments seems quite difficult to follow, how about def self.new(*attrs, keyword_init: nil, &block)
and then below check if attrs.first
is a Symbol (if so it's an attribute, so nothing special to do, if not try StringValue and if that succeeds use it as the name and .shift
).
Another advantage of that is the common case by far is no klass_name, so then we avoid an extra unshift for that case + make the logic clearer.
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.
Oh, my bad. Sorry for such delaying. The PR will be merged ASAP.
PullRequest: truffleruby/4246
Ruby 3.3 introduces a new 0 args constructor for Struct. This PR implements that compatibility in Truffleruby.