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

Improves api consistency related to modules containing the generated protobuf assets #1221

Closed
tottoto opened this issue Jan 7, 2023 · 3 comments · Fixed by #1224
Closed

Comments

@tottoto
Copy link
Collaborator

tottoto commented Jan 7, 2023

Feature Request

Crates

  • tonic-types
  • tonic-health
  • tonic-reflection

Motivation

Improves apis consistency related to modules containing the generated protobuf assets.

Proposal

Warning: This proposal includes breaking changes.

In this discussion, I assume the project status of 0ca0630. Currently we have some inconsistency apis in these crates.

tonic-types tonic-health tonic-reflection
module name of protobuf pb proto proto
variable name of encoded file descriptor set FILE_DESCRIPTOR_SET GRPC_HEALTH_V1_FILE_DESCRIPTOR_SET FILE_DESCRIPTOR_SET

Regarding to the module name of protobuf, my proposal is to rename all module names to pb or proto. My preference is pb as pb might better represent protobuf. proto is a common word so I think pb is a more appropriate module name.

When we uses the encoded file descriptorset (for example: FILE_DESCRIPTOR_SET in tonic-types), we uses register_encoded_file_descriptor_set. However there is register_file_descriptor_set. these apis might be confusable. In this point, my proposal is to rename the current variable name of the encoded file descriptor set to ENCODED_FILE_DESCRIPTOR_SET and provide new FILE_DESCRIPTOR_SET which is the FileDescriptorSet.

Alternatives

Leaves these apis inconsistency. However we might be able to take these proposals partially. For example, renaming GRPC_HEALTH_V1_FILE_DESCRIPTOR_SET in tonic-health to FILE_DESCRIPTOR_SET can be introduced without breaking changes by just being added as a new variable.

@LucioFranco
Copy link
Member

Yeah, I think changing them all to use pb and rename the FDS to be FILE_DESCRIPTOR_SET is also a good change. Both seem good to me 👍

@tottoto
Copy link
Collaborator Author

tottoto commented Jan 9, 2023

Thanks for your review! I'll work on this.

@tottoto
Copy link
Collaborator Author

tottoto commented Jan 9, 2023

I noticed that we can not get prost_types::FileDescriptorSet as static variable since prost::Message::decode is a non-const fn. So I'll change all file descriptor set variable names to FILE_DESCRIPTOR_SET which contains byte encoded value.

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 a pull request may close this issue.

2 participants