Skip to content
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

Changed JSON parser/printer to correctly camelCase names. #46

Merged
merged 3 commits into from
Feb 18, 2016

Conversation

haberman
Copy link
Member

This required adding support for json_name (and its camelCase transformation from the regular name) to upb_fielddef.

For now we are not adding a json_name -> fielddef* map to upb_msgdef. In many/most cases this will never be used. Instead we add the map to the JSON parser's data it creates up-front.

@haberman
Copy link
Member Author

Review to @scwhittle.

@@ -721,6 +722,35 @@ const char *upb_fielddef_name(const upb_fielddef *f) {
return upb_def_fullname(upb_fielddef_upcast(f));
}

bool upb_fielddef_getjsonname(const upb_fielddef *f, char *buf) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this a method on fielddef instead of a different method taking any const char* name and camel casing?

If it has to be a method on fielddef, perhaps separate the transformation to a separate helper method that might be useful elsewhere for json

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems safer to pass in the size of buf and perform checks on that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Camel casing the regular name is the default for the JSON name, but it can also explicitly be set to something else. See: https://github.com/google/protobuf/blob/master/src/google/protobuf/descriptor.proto#L193

Separating out as a utility function could possibly be useful, but I'd rather late-bind on that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done on your second suggestion.

@haberman
Copy link
Member Author

Addressed comments, PTAL.

@haberman
Copy link
Member Author

Added asserts, PTAL.

@scwhittle
Copy link

LGTM

haberman added a commit that referenced this pull request Feb 18, 2016
Changed JSON parser/printer to correctly camelCase names.
@haberman haberman merged commit 32236c9 into protocolbuffers:master Feb 18, 2016
@haberman haberman deleted the jsoncamel branch February 18, 2016 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants