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(build): Reexport types related to prost #1925

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

tottoto
Copy link
Collaborator

@tottoto tottoto commented Sep 5, 2024

Reexports types which are used with builder based on prost.

Comment on lines 84 to 87
#[cfg(feature = "prost")]
pub use prost_build::Config as ProstBuildConfig;
#[cfg(feature = "prost")]
pub use prost_types::FileDescriptorSet;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these already used in tonic-build's public API? Where/how?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

prost_build::Config is used by compile_{protos, fds}_with_config builder APIs and prost_types::FileDescriptorSet is used by compile_fds{_with_config} builder APIs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that Config is used as Config in those functions and given that there is no type definition called Config anywhere in the tonic repo, IMO it would be clearer to just import Config without aliasing it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looking at the current API, it can be interpreted that prost_build::Config is adopted as the config for tonic-build, so there seems to be no misunderstanding if tonic-build reexport it as Config. Changed to reexport prost_build::Config as Config.

@tottoto tottoto force-pushed the reexport-types-related-to-prost branch from 4050277 to f000285 Compare September 19, 2024 14:49
@tottoto tottoto added this pull request to the merge queue Sep 20, 2024
Merged via the queue into hyperium:master with commit ffd7794 Sep 20, 2024
17 checks passed
@tottoto tottoto deleted the reexport-types-related-to-prost branch September 20, 2024 11:50
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.

2 participants