Ruby: Add support for proto3 json_name in compiler and field definitions#8356
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
3f6da46 to
2c354c6
Compare
|
@haberman I assume you would be the correct person to review this further? (Thanks for your work on the Ruby protobuf support!) |
|
@haberman Ping on this, tagging you since you seem to be the primary maintainer for the Ruby bindings - not sure if I missed anything here. Appreciate the help - thank you :) |
|
Hi @lfittl , I'm sorry for my slow reply on this. I'm somewhat reluctant to go farther down this path of extending the DSL, because it is an un-winnable game of whack-a-mole. The DSL fundamentally can't represent all options, particular custom options that are used in their own definition: That said, the fact that |
| FieldDescriptor* self = ruby_to_FieldDescriptor(_self); | ||
| const upb_fielddef *f = self->fielddef; | ||
| const char *json_name = upb_fielddef_jsonname(f); | ||
| if (json_name != NULL) |
There was a problem hiding this comment.
I don't think this check should be necessary, upb_fielddef_jsonname() should never return NULL.
| rb_hash_lookup(options, ID2SYM(rb_intern("json_name"))); | ||
|
|
||
| /* Call #to_s since json_name is always a string in the descriptor. */ | ||
| json_name = rb_funcall(json_name, rb_intern("to_s"), 0); |
There was a problem hiding this comment.
I don't follow, if it is a string already why do we need to call to_s on it?
There was a problem hiding this comment.
Yup, you are correct - this was copied from the default value handling above, but json_name is indeed always a string, so this conversion is unnecessary.
No worries, I know the same from my own open source projects, thanks for taking a look!
Yeah, I see how it's important to handle this generically at some point - but I think it still makes sense to do this now, since it allows use of I've addressed the review comments you shared, and also added a test that covers this, to ensure it doesn't break by accident in the future. |
* Ruby: Add support for proto3 json_name in compiler and field definitions * Address review feedback * Add test for json_name functionality Co-authored-by: Lukas Fittl <lukas@fittl.com>
This was previously relying on an auto-generated mapping layer, but since the upstream patch was merged into protobuf 3.17.0 we can rely on the upstream support instead. See protocolbuffers/protobuf#8356
This was previously relying on an auto-generated mapping layer, but since the upstream patch was merged into protobuf 3.17.0 we can rely on the upstream support instead. See protocolbuffers/protobuf#8356
…-for-ruby Ruby: Add support for proto3 json_name in compiler and field definitions
This adds support for the
json_namefield definition, per the Proto3 spec.Note the JSON output mechanism is already reading the
json_namefrom ubp's struct (see here), but this was not being added to the generated Ruby DSL, or read in from the DSL into the ubp field definitions.