-
Notifications
You must be signed in to change notification settings - Fork 15.6k
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
Ruby: Add support for proto3 json_name in compiler and field definitions #8356
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 |
ruby/ext/google/protobuf_c/defs.c
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this check should be necessary, upb_fielddef_jsonname()
should never return NULL.
ruby/ext/google/protobuf_c/defs.c
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
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_name
field definition, per the Proto3 spec.Note the JSON output mechanism is already reading the
json_name
from 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.