-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add partial aio typing stubs #33
Conversation
@shabbyrobe any thoughts on this PR? The tests should provide the Minimal Reproducible Examples that you need to evaluate the contribution. |
Thank you for the contribution, and than you for the reminder. I suspect I need to clarify the language about minimal reproducible examples: they're so I can run code that exercises a contribution locally, myself. There's really no substitute for that, especially for major changes. The typing tests are great, but they aren't enough. Given I've had to revert disruptive |
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.
Looks good! Just a few things to address around FIXMEs and typing.Any. With those minor fixes and a proper MRE, I'd be happy to merge this in. Thank you again!
Added an MRE (basically a direct copy of the test from nipunn1313/mypy-protobuf#489). Looking at your comments. |
I have an idea: what about replacing Any here (and in other partially typed
signatures) with an uninhabited type? Then all usage of that type would
need an explicit ignore, which punts safety to the user and allows these
stubs to evolve in the correct direction later.
Wdyt?
…On Sat, 4 Mar 2023, 10:20 Blake Williams, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In grpc-stubs/aio.pyi
<#33 (comment)>:
> +from concurrent import futures
+from types import TracebackType
+from . import (
+ _Options,
+ Compression,
+ GenericRpcHandler,
+ ServerCredentials,
+ StatusCode,
+ ChannelCredentials,
+ CallCredentials,
+)
+
+"""Create Client"""
+
+# FIXME
+ClientInterceptor = typing.Any
I think I take a different default stance on typing.Any. If we get it
wrong, it risks introducing a breaking change in the typings later. If we
leave it typing.Any, it guarantees introducing a breaking change in the
typings later.
I need more convincing here.
—
Reply to this email directly, view it on GitHub
<#33 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPMM3XKNVVVZXK6H3CVNMTW2KDDFANCNFSM6AAAAAAUWM6BYQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
That sounds like it could solve both our problems! How would that look? |
Looks good! Let's go with it. Thank you for the quick turnaround for those tweaks, and thank you for the MRE, it will help a lot if any issues arise. |
Just about to release to pypi, wondering how we version this. It could be breaking for folks consuming it but expecting aio to be untyped. I was originally tracking the grpc version itself, but I wasn't super disciplined there and became unmoored from that some time ago. Split versioning of separate types and the original library is, to my mind, an unsolved problem. I still ultimately want it to be clear which version of grpc the typings are based on. The typings need a major version and a patch version too, with semver-lite semantics. I wonder if maybe it needs to be versioned like this: |
I think the major and minor version of grpc-stubs should match the grpc package. IMHO a separate grpc-stubs "major" version isn't necessary, since if there were any future massive type changes (although not sure how that would play out while still being compatible with grpc) then I think a whole new package/fork would be better than a bumped major version. Keep a patch version independent of grpc for fixes in the stubs, but honestly I think that's sufficient control here. As for the unmoored version of grpc vs grpc-stubs, I'm not sure it matters too much. A user will see grpc-stubs has an older minor version, and will likely deduce that some new features in grpc will be incorrectly typed but existing features will remain compatible (as compatible as type stubs can be at least). I would update the minor version of grpc-stubs to match grpc each time grpc-stubs releases, i.e. the minor version will likely be sparse and have gaps. |
Thanks for you perspective. I'll skip on the "typing major" concept for now but I'll keep it in my pocket. There are still some problems I think it has the potential to solve, but I'll wait until I've seen instances of them happen in practice first. I will go with a sub-patch version for typing updates from now on though, and I'll publish now. Thanks again for your contribution, it has been a gap for a while.
They should, but it needs a once-over to make sure it still lines up with the docs. |
Description of change
Added typing stubs for:
This should be sufficient for basic mypy-protobuf usage.
Also added a test for multi-callable usage, since that's the gnarliest part.
Minimum Reproducible Example
dummy_pb2_grpc.py
dummy_pb2_grpc.pyi
dummy_pb2.py
dummy_pb2.pyi
test_grpc_async_usage.py
run.sh
Checklist:
typesafety/test_grpc.yml
for all APIs added or changed by this PRmypy-protobuf
.