-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: grpc runtime #819
feat: grpc runtime #819
Conversation
fa49397
to
c316cd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 💪
PS: If you find any way to lighten the addition of new runtimes, feel free to write an RFC ;-)
Can you update the description? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests required!
9942338
to
30d1470
Compare
next step, auto gener type for input
next step refactoring
it's not yet perfect but it's good start
30d1470
to
f79375e
Compare
generate type from proto_file_content
fix error: about missing op_grpc_*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sort of get why you went the parser way but handling things like import resolution for example is not an easy task. Adapting protobuf_parser to fit our needs might solve this. wdyt?
common.proto
syntax = "proto3";
package example;
message User {
string name = 1;
int32 id = 2;
}
main.proto
syntax = "proto3";
package example;
import "common.proto"; // <= here
service UserService {
rpc listAll(User) returns (User);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be merged as a first iteration👍
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great 👍
Waiting for the tests to pass...
Migration notes
...