Include proto macros #13
Conversation
tonic-build/src/lib.rs
Outdated
| .out_dir | ||
| .clone() | ||
| .unwrap_or_else(|| PathBuf::from(std::env::var("OUT_DIR").unwrap())); | ||
| if self.out_dir.is_none() { |
There was a problem hiding this comment.
I'm not sure I understand why this was changed?
There was a problem hiding this comment.
The builder is taken by self by the ServiceGenerator to access it's fields. I needed to access the out_dir field inside of the ServiceGenerator::finalize method but it was not being set on the builder itself. So I modified the code here to set the out_dir field on the builder.
There was a problem hiding this comment.
Ah but why not use unwrap_or_else?
tonic/src/macros.rs
Outdated
| #[doc(hidden)] | ||
| #[macro_export] | ||
| macro_rules! include_helper { | ||
| ($include: ident, $ending:tt) => { |
There was a problem hiding this comment.
I'd prefer if $include took a string/str instead since I feel that feels more natural to what it is actually doing. An ident makes me think of something that exists already in the code.
| } | ||
| use tonic::include_client; | ||
|
|
||
| include_client!(helloworld, hello_world); |
There was a problem hiding this comment.
So I still kinda prefer
pub mod pb {
include_client!("helloworld");
}thoughts? This removes the need to support renaming since you can do it yourself and also can just include it directly without a module.
There was a problem hiding this comment.
Sure! It seems more explicit that way, which is probably a good thing.
tonic/src/macros.rs
Outdated
| #[macro_export] | ||
| macro_rules! include_client { | ||
| ($name: tt) => { | ||
| $crate::include_helper!($name, ".rs"); |
There was a problem hiding this comment.
what happens if someone does
include_server!(..);
include_client!(..);That wouldn't compile because you're redefining the proto each time?
There was a problem hiding this comment.
Yes, it would import the {package}.rs file twice and cause conflicts.
There was a problem hiding this comment.
I wonder how we could avoid this, I feel like this will cause issues with people who are trying tonic out :(
There was a problem hiding this comment.
I don't think there is a way to do it w/ two macros. We could possibly try something else?
include_proto!(helloworld);
include_proto!(helloworld as hello_world);
include_proto!(helloworld, features = [server]);(just throwing ideas out, i don't know if they are good or not)
There was a problem hiding this comment.
@carllerche definitely helpful ideas! I think that the following design should fix the concerns raised. Where the features expression can be equal to client and/or server and is both by default if omitted.
pub mod hello_world {
include_proto!("helloworld", features = [server]);
}
tonic/src/macros.rs
Outdated
| }; | ||
| } | ||
|
|
||
| /// Include generated proto client items. |
There was a problem hiding this comment.
I say we drop include_server and include_client for now.
There was a problem hiding this comment.
They should be removed already
Includes 3 new macros as mentioned in #1.
include_proto!include_client!include_server!Example:
is now replaced by the following in the server example.
Optionally an included module can be renamed via a second argument to any of the macros.