twitchtv / twirp

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

fix ./example/ to be in sync with ./docs/example.md #355

Closed ra1u closed 2 years ago

ra1u commented 2 years ago

./example is modified in a way to be same as docs/example.md

There are some modifications to docs/example.md , particularly to command protoc that was incompatible with recent versions of protoc.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

wmatveyenko commented 2 years ago

Hi, can you elaborate a bit more on the problem you are seeing? It would be helpful to know what you saw versus what you expected, what version of protoc you were using, etc.

Thanks, Wade

ra1u commented 2 years ago

Runing as per https://twitchtv.github.io/twirp/docs/example.html#generate-code protoc --twirp_out=. --go_out=paths=. rpc/haberdasher/service.proto

returns paths=./: No such file or directory

Versions:

Installed as per https://twitchtv.github.io/twirp/docs/install.html#install-twirp-and-protobuf-generators

go install github.com/twitchtv/twirp/protoc-gen-twirp@latest
go install google.golang.org/protobuf/cmd/protoc-gen-go@latest

protoc-gen-go

protoc-gen-go --version  
protoc-gen-go v1.28.0

protoc-gen-twirp

protoc-gen-twirp  --version 
v8.1.2

./check_protoc_version.sh

protoc version: libprotoc 3.18.1  
wmatveyenko commented 2 years ago

Sorry this took so long. Please fix the conflict and then we can merge.

wmatveyenko commented 2 years ago

Hi @ra1u, just pinging to see if you still want this PR to be merged.

ra1u commented 2 years ago

I am sorry. I can take time to ressurect this in next days or over this weekend.

ra1u commented 2 years ago

Main idea here is having coherent example and documentation. I am still not sure this is all correct, so I am looking for someone to take review.

wmatveyenko commented 2 years ago

Looks good. Thanks for your contribution!

wmatveyenko commented 2 years ago

Hi, we had to revert this change, as the tests failed after this was merged. If you want to resubmit with fixed tests, that would be great.