dsymonds / gotoc

Protocol Buffer compiler written in Go
Other
120 stars 8 forks source link

Add stream support to rpc. #3

Closed sbunce closed 7 years ago

sbunce commented 8 years ago

Add support for client and server streaming.

Example:

service Serv {
    rpc Foo(stream Bar) returns (stream Baz);
}
sbunce commented 8 years ago

David, My goal is to use gotoc with grpc. Any pointers on what needs to get done are appreciated. Thanks!

Seth

sbunce commented 8 years ago

I thought of a problem with this CL. There may be a message called "stream". I'll think about how to handle this.

message stream {}

service Serv {
    rpc Foo(stream) returns (stream);
}
sbunce commented 8 years ago

I was able to resolve that ambiguity. It now handles the corner case where a message is called "stream". I added a test for it. The code is more complicated but I can't think of a way to make it simpler.

message stream {}

service Foo {
rpc Bar(stream) returns (stream);
}
dsymonds commented 8 years ago

Yeah, this looks too complicated. There's other parts of the parser that effectively do a token lookahead. I think you can just parse the token, and if it is the literal "stream" then the next token is either the stream's message name or ")". Alternatively, read two tokens (there always has to be at least two left), and it'll either be "stream" and Msg, or Msg and ")".

dsymonds commented 8 years ago

Also, please squash the commits when you push a new version.

sbunce commented 8 years ago

I checked in some garbage by accident. Trying to fix now. Please ignore last commit.

sbunce commented 8 years ago

I fixed my bad git push. Sorry for that noise.

I took a look at other parts of the parser where you use p.back(). I refactored this change to use p.back() and a few conditions. I think it's simpler to understand now.

dsymonds commented 8 years ago

Sorry I've been slow. I'll try to get to it this weekend.

dsymonds commented 7 years ago

Yes, please just squash and force push. The GH review system is still horrid. I only want to merge a single commit, and this change is small enough to re-review in total each time.

sbunce commented 7 years ago

I squashed and force pushed.

sbunce commented 7 years ago

Thanks for your patience on this review. I should have noticed that extraneous else branch.

I pushed an updated version.

dsymonds commented 7 years ago

Merged in 6e9e04d.

Thanks for your contribution!