-
Notifications
You must be signed in to change notification settings - Fork 15.4k
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
Fixed to_h with repeated messages to return hashes in Ruby #3639
Fixed to_h with repeated messages to return hashes in Ruby #3639
Conversation
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure. |
1 similar comment
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure. |
Can one of the admins verify this patch? |
ok to test |
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 for the change! Just one small readability thing.
ruby/ext/google/protobuf_c/message.c
Outdated
@@ -419,6 +419,13 @@ VALUE Message_to_h(VALUE _self) { | |||
msg_value = Map_to_h(msg_value); | |||
} else if (upb_fielddef_label(field) == UPB_LABEL_REPEATED) { | |||
msg_value = RepeatedField_to_ary(msg_value); | |||
|
|||
if (upb_fielddef_msgsubdef(field) != NULL) { |
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 it would be a bit clearer to write:
if (upb_fieldddef_type(field) == UPB_TYPE_MESSAGE)
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.
Makes sense! I fixed that.
8a06394
to
83264bd
Compare
Calling
to_h
converts everything to a Ruby object representation, except if you have a repeated field with a message, then you get the message field object. This fixes it so it converts that to a hash too (and recursively does it).Calling
to_h
in JRuby didn't work if you had a repeated field, which is fixed in this too (mostly incidentally).RubyRepeatedField
includesRubyEnumerable
which responds toto_h
, so it was trying to callto_h
on an array, which errored.to_h
in JRuby with a map field is still broken, because it converts it to a key/value representation where value is the proto object not the hash representation. Didn't fix that here, because it was unrelated to fixing repeated messages.I think it likely makes more sense if
RepeatedField_to_ary
andRubyRepeatedField#toArray
did this, but I was opting to make less dramatic changes.