Skip to content

feat: honor json_name option#1825

Open
michaelbgreen wants to merge 5 commits into
protobufjs:masterfrom
michaelbgreen:master
Open

feat: honor json_name option#1825
michaelbgreen wants to merge 5 commits into
protobufjs:masterfrom
michaelbgreen:master

Conversation

@michaelbgreen

Copy link
Copy Markdown

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
Copy Markdown

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

@aneeshgarg

Copy link
Copy Markdown

Will this change also use json_name for files generated statically?

@tomquist

Copy link
Copy Markdown

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

@YashJainSC

Copy link
Copy Markdown

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

@tomquist

Copy link
Copy Markdown

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

@mbohal

mbohal commented Nov 28, 2023

Copy link
Copy Markdown

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

@alexander-fenster alexander-fenster left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown

+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

tomquist commented Dec 9, 2023

Copy link
Copy Markdown

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

@aneeshgarg

Copy link
Copy Markdown

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

@alexander-fenster

Copy link
Copy Markdown
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
Copy Markdown
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