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

Add support for 'json_name' annotation. #210

Closed
wants to merge 1 commit into from
Closed

Conversation

fxb
Copy link
Contributor

@fxb fxb commented Feb 10, 2021

This PR adds support for the json_name annotation.

I went through calls to maybeSnakeToCamel and replaced it with calls to a new function called getFieldName, which will additionally inspect the jsonName field of FieldDescriptorProto.

A simple integration test is added as well.

I decided to not add a configurable option for this right now, but it would be trivial to do so.

According to the proto3 language guide parsers should to handle different field names:

Parsers accept both the lowerCamelCase name (or the one specified by the json_name option) and the original proto field name.

As this currently does not seem to be implemented, I also skipped it for the json_name support.

Copy link
Contributor

@marcushultman marcushultman left a comment

Choose a reason for hiding this comment

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

🎉

export const protobufPackage = 'simple';

export interface Simple {
other_name: string;
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, my interpretation of json_name attribute would be that this particular name would still be name and then other_name would only show up on the JSON on the wire (i.e. in toJson methods) and then looked for when data is parsed in from the wire (i.e. in the fromJson methods).

Can we do that instead?

Base automatically changed from master to main March 14, 2021 15:29
@stephenh
Copy link
Owner

Fixed in #408

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants