feat: support async_trait-less generation
#1697
Conversation
jun-sheaf
force-pushed
the
master
branch
2 times, most recently
from
03921c4
to
9b03433
Compare
jun-sheaf
force-pushed
the
master
branch
3 times, most recently
from
4e862a5
to
4ea070c
Compare
|
@LucioFranco Note that the current CI doesn't test all feature combinations. We should use |
tonic-build/src/server.rs
Outdated
| async fn #name(#self_param, request: tonic::Request<#req_message>) | ||
| -> std::result::Result<tonic::Response<#res_message>, tonic::Status> { | ||
| Err(tonic::Status::unimplemented("Not yet implemented")) | ||
| #[cfg(not(feature = "async_trait"))] |
There was a problem hiding this comment.
That's a lot of code duplication on top of what's already there. Given that the only difference is whether the method is prefixed with async or not and wrapping of the return type in impl std::future::Future<Output = ..>, can we simplify?
There was a problem hiding this comment.
Decomposing a method to such an extent may not be advisable. It represents a premature optimization that could lead to developer confusion and complications. Understanding the interdependencies among the components within the decomposition requires extra caution, potentially increasing the risk of errors.
There was a problem hiding this comment.
That's not a satisfying answer to me. I don't think the term "premature optimization" applies here, since there's no performance impact to what I'm suggesting (indeed, the performance might well be worse).
I'm suggesting that the increased code duplication makes this code harder to maintain (for example, carefully reviewing these changes is a pain in the ass), and asking you whether that issue could be mitigated.
There was a problem hiding this comment.
The changes have already been made. I was just expressing an opinion.
Motivation
async_traitis not necessary for most use-cases. This PR provides the option for users.Solution
Providing an option to generate async traits without
async_trait.Fixed: #1651