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

Audit initial main crate API #1

Closed
LucioFranco opened this issue Sep 18, 2019 · 5 comments
Closed

Audit initial main crate API #1

LucioFranco opened this issue Sep 18, 2019 · 5 comments

Comments

@LucioFranco
Copy link
Member

The main crate's api needs to be audited, the transport module is still under dev so probably not worth it right now to give that a thorough review but would be nice to get a light review of it.

@carllerche
Copy link

Building

  • Conventional proto layout to avoid having to list out all files to
    compile. Ideal build.rs file would only contain tonic_build::compile_protos().

Examples: general

  • instead of having to manually include files, it would be nice to be
    able to do:

    `include_proto!("helloworld");`
    

    This could potentially be achieved by generating .rs files at
    $OUT_DIR root matching the service names and then transforming
    include_proto!($name) -> what you are doing now.

  • We may want to generate client & server definitions into separate
    files? I am not sure. Then it could be:

    include_client!("hello_world");
    include_server!("helo world");

Example: Greeter client

  • Greeter example should be: GreeterClient::connect("http://[::1]:50051").
    There should be a "default transport" that comes pre-configured.
  • At the risk of beating a dead horse, I really wish we didn't have to
    manualy wrap messages w/ Request::new.

Example: Routeguide server

  • tokio::spawn instead of thread::spawn.

Lib

  • Remove all generic grpc stuff for now (docs reference Generic).
  • In general, any APIs made public for macros should be do hidden.
    • As such, questions about "why is __ public?" means why is it
      intended to be used directly by the user of tonic.
  • Why is body module public?
  • Why is tonic::server public?
  • Why is tonic::codec public?
  • Why is tonic::client public?

@carllerche
Copy link

Codegen: anyway to run output through rustfmt?

@carllerche
Copy link

Generated client

It looks like you are exposing async fn ready. I would probably make that part of the individual methods. If there is a need to expose later, we can.

@carllerche
Copy link

Generated server

Does GreeterServerSvc need to be shown in docs or can it be hidden?

@hawkw
Copy link
Contributor

hawkw commented Sep 23, 2019

What do you think about changing the client API so that instead of

    let request = tonic::Request::new(HelloRequest {
        name: "hello".into(),
    });

    let response = client.say_hello(request).await?;

users could just write

    let request = HelloRequest {
        name: "hello".into(),
    };

    let response = client.say_hello(request).await?;

We could add an Into<Request<Self>> implementation to the generated request messages, and change the generated RPC methods on the client to be like

async fn #ident(&mut self, request: impl Into<tonic::Request<#request>>) 

Since all T implements Into<T>, users could still construct tonic::Request types if needed, such as when manually adding headers etc.

This is admittedly a very minor papercut, but it seems like it would reduce some friction in the APIs that I suspect a new user is most likely to use (and make the basic examples seem simpler!)...

Something similar could probably be done for response messages.

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

No branches or pull requests

4 participants