-
Notifications
You must be signed in to change notification settings - Fork 56
Unimplemented plugin #593
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
Unimplemented plugin #593
Conversation
buf.add("") | ||
buf.add("") | ||
buf.add("class Unimplemented{}Base({}Base):", service.name, service.name) | ||
with buf.indent(): | ||
for name, _, request_type, reply_type in service.methods: | ||
buf.add("") | ||
buf.add( | ||
"async def {}(self, stream: '{}.{}[{}, {}]') -> None:", | ||
name, | ||
server.__name__, | ||
server.Stream.__name__, | ||
request_type, | ||
reply_type, | ||
) | ||
with buf.indent(): | ||
buf.add("raise grpclib.exceptions.GRPCError(grpclib.const.Status.UNIMPLEMENTED)") |
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.
These are the lines I added compared to the grpclib version
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 tested the new plugin by removing all the unimplemented methods from the RobotService
. Everything still works as expected, and everything typechecks.
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.
This is amazing; I didn't know this could be done. 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.
LGTM! Just for my own understanding, are there any concerns around important upstream changes being lost such that pushing this upstream becomes higher priority?
Not really. The last time this file was updated was 10 months ago, and then 2 years ago, and then 4 years ago. So I don't think we lose much by bringing this in house. |
grpclib in python is not forwards compatible (see issue).
This was getting really annoying for people who didn't make gRPC changes, but had to deal with other people's changes because tests would fail.
So I made a protoc plugin that's the grpclib plugin with an added bit that will automatically generate an
UnimplementedServiceBase
similar to how it happens in golang.This generated a fair few additional lines of code, but it's all generated boilerplate.
You still have to opt-in to using the unimplemented version by subclassing from it. Otherwise everything continues to work as normal. I'm considering making a PR upstream for this