search

twitchtv / twirp

A simple RPC framework with protobuf service definitions
https://twitchtv.github.io/twirp/docs/intro.html
Apache License 2.0
7.2k stars 326 forks source link

Wiki example content issues #24

Closed andyleap closed 6 years ago

andyleap commented 6 years ago

There are several code issues on the https://github.com/twitchtv/twirp/wiki page. While nothing is advertising those snippets as 100% perfect, it'd be nice to have the egregious errors fixed. Notably:

Server snippet:

func (s *HelloWorldServer) Hello(ctx context.Context, req *pb.HelloReq) (hat *pb.HelloResp, error) {

the return values mix named and unnamed returns, which is unallowed. Consider removing the "hat" name.

Client snippet

The client created is targeting port 8000, where the server was put on port 8080.

The client is also doing reversed logic err check:

if err != nil {
    fmt.Println(resp.Text) // prints "Hello World"
}

which should only print anything if an error was returned, which is almost assuredly the wrong thing to be doing here.

In addition, I'd consider changing resp, err = client.Hello( to resp, err := client.Hello(, as there's nothing in this snippet creating resp or err, so it would seem cleaner this way

spenczar commented 6 years ago

Thanks for pointing these (indeed egregious) errors out. I've now put in code that I've verified actually compiles.

andyleap commented 6 years ago

It does now compile, but it still does not work, as the client and the server are using different ports, and you only print the response if the rpc returns an error....

spenczar commented 6 years ago

Sheesh! This will be a lot better to keep up with when we have docs in a place that can get contributions from the outside world. Sorry for the runaround, but thank you for being diligent.