-
Notifications
You must be signed in to change notification settings - Fork 15.5k
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 initializing a chain of protos using only a hash in Ruby #3627
Allow initializing a chain of protos using only a hash in Ruby #3627
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? |
4f0d23c
to
399ab27
Compare
ok to test |
Looks like this change failed Travis? |
399ab27
to
8771483
Compare
@haberman Fixed the test that failed. It was one of the compatibility tests, which now fails with Is the fact that the exception changed ok? |
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 this change. I think the changed exception should be ok. I wouldn't expect anyone to be catching this exception, and the new behavior seems more consistent anyway.
ruby/ext/google/protobuf_c/message.c
Outdated
@@ -248,16 +260,26 @@ int Message_initialize_kwarg(VALUE key, VALUE val, VALUE _self) { | |||
Map_merge_into_self(map, val); | |||
} else if (upb_fielddef_label(f) == UPB_LABEL_REPEATED) { | |||
VALUE ary; | |||
VALUE entry; |
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.
This is only used inside the for loop, so please declare it there.
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!
I saw #3120 also asked for this. We're in the process of trying to migrate people from a pure Ruby proto gem to the Google one, and the requirement to initialize all protos is a blocker. Sounds like there was interest in this, so figured I'd take a stab at implementing it in.
Taking the examples from #3120, this lets you do:
MyMessage.new(submessage_field: { foo: 42 })
and it will automatically turn the{ foo: 42 }
into aSubMessage.new(foo: 42)
.Since a common scenario is also to be turning JSON into protos (for proto backed JSON APIs), I also changed the initialization to support symbol or string keys to prevent having to do a deep symbolization pass before initializing the protos.