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

feat: honor json_name option #1825

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

michaelbgreen
Copy link

Honor the json_name option. This is useful when a .proto file is compiled for multiple languages, to conform to whatever random casing rules were chosen for the project:

  • The .proto convention is to use lower_snake_case for field names, and UPPER_SNAKE_CASE for enum names
  • C++ (via protoc) simply uses lower_snake_case for field names
  • C# (via protobuf-net) uses PascalCase for both field and enum names, and has options for customizing the names
  • JavaScript (via protobuf.js) uses camelCase for field names, and the json_name attribute can now be used to customize the name

Example:

message NetworkAdapter {
  string get_ip_address = 1 [json_name = "getIPAddress", (.protobuf_net.fieldopt).name ="GetIPAddress"];
}

Default names without the options:

  • C++: get_ip_address (unchanged)
  • C#: GetIpAddress
  • JS: getIpAddress

Fixes #992, fixes #1245, fixes #1775

Michael Green added 3 commits October 12, 2022 11:09
…iled for multiple languages, to conform to whatever random casing rules were chosen for the project:

- The .proto convention is to use lower_snake_case for field names, and UPPER_SNAKE_CASE for enum names
- C++ (via protoc) simply uses lower_snake_case for field names
- C# (via protobuf-net) uses PascalCase for both field and enum names, and has options for customizing the names: https://github.com/protobuf-net/protobuf-net/blob/main/src/Tools/protogen.proto
- JavaScript (via protobuf.js) uses camelCase for field names, and the json_name attribute can now be used to customize the name
Example:
message NetworkAdapter {
  string get_ip_address = 1 [json_name = "getIPAddress", (.protobuf_net.fieldopt).name ="GetIPAddress"];
}
Default names without the options:
- C++: get_ip_address (unchanged)
- C#: GetIpAddress
- JS: getIpAddress
@aneeshgarg
Copy link

Can we prioritize this review? We need it as well.

@aneeshgarg
Copy link

Will this change also use json_name for files generated statically?

@tomquist
Copy link

Any update on this? Would really like to see this land.

@YashJainSC
Copy link

it seems we have a solution here for this
#564 (comment)

@tomquist
Copy link

The linked comment is not exactly a solution as it doesn't respect the explicitly defined json_name.

@mbohal
Copy link

mbohal commented Nov 28, 2023

Is there any way I could help this PR move forward and get merged?

I just did a quick test against v7.2.5 and it seems to be working. I really need this functionality and I would like to to avoid having to maintain my own fork.

@alexander-fenster alexander-fenster self-requested a review December 8, 2023 08:17
@alexander-fenster alexander-fenster changed the title Honor json_name option feat: honor json_name option Dec 8, 2023
Copy link
Contributor

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

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

The code looks good to me, my main concern here is that it's technically a breaking change. The code that used the original field names will stop working if the proto defines json_name for those fields. I feel it's a little bit more serious than https://xkcd.com/1172/ so I would rather stay on the safe side and call it a major version.

What do you folks think if we mark it as a breaking feature, and combine it with dropping Node.js v12 support to release it as 8.0.0? I'll wait for a few days here for any feedback from the community.

@aneeshgarg
Copy link

+1 to releasing it as a major version. This change will definitely break my current use case.

Can this be behind a build flag/option?

@tomquist
Copy link

tomquist commented Dec 9, 2023

Alternatively this could be a runtime option which is off by default.

@aneeshgarg
Copy link

Runtime option might not work with folks like us using the statically generated files.

@alexander-fenster
Copy link
Contributor

I think it can technically go to IParseOptions (right near the keepCase) and to the CLI flag of pbjs for static proto generation. That way we can make it false by default and avoid making a semver major version. Not sure if @michaelbgreen has time to implement it that way though :) (I might have but later this month).

@michaelbgreen
Copy link
Author

I am happy to see that this is getting traction, but sorry, I no longer have time to contribute to this.

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