-
Notifications
You must be signed in to change notification settings - Fork 249
Add stub typing for clients #1075
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
base: master
Are you sure you want to change the base?
Add stub typing for clients #1075
Conversation
These type stubs are being used in a couple of my projects:
|
…ype cheeck against stub files.
edf33a5
to
f2fe1eb
Compare
I tried these stubs against Python 3.9.21 by copying *.pyi to my stubs directory. However even with
I've tried different python and mypy versions and the cause seems to be using As a workaround, this seems to work:
|
|
||
__version__ = ... | ||
__all__ = [ | ||
"AIOKafkaConsumer", |
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.
The top-level __init__.pyi
should import and expose AIOKafkaProducer
in __all__
.
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.
Good catch. I think I started by typing the consumer and left this out initially, and forgot to put it back in. Fixed.
Thanks for identifying this. I use PyRight as my type checker, as MyPy doesn't seem to understand types under certain circumstances. But I understand that that's the main type checker people use, and your workaround looks like a great solution. I'm going to just make sure it doesn't upset PyRight (I don't think it will), and update it shortly. |
Unfortunately, PyRight did not like this workaround. PyRight was smart enough to realize the type variable wasn't actually needed in the identity functions, and rejected it:
I also tried You can probably tell, but what I was trying to do here is to have the default producer be an
You can also explicitly override the type by providing the appropriate serializer functions. I have an application with an I'm going to try and spend some time this weekend to see if I can come up with a solution that is sound (and therefore type-checks in PyRight), while also conforming to MyPy's sensibilities. |
…ing safety for proper type-checkers like pyright).
For the problem with Prompt:
AI Response:
Prompt:
AI Response:
|
Finally, to ensure the use of contra-variance is appropriate here: Prompt:
AI Response:
|
Could we make key_serializer and value_serializer |
That's how the serializers were originally implemented, so it's definitely possible at the cost of type safety. But the purpose of this PR was to improve type safety, and really it has. The problem is that there is a bug in mypy when dealing with generic types and default arguments. Having said that, I'm not opposed to "being explicit rather than inferring the type", and if we wish to do that, we can simply omit the default arguments, and require mypy users to explicitly provide them. However, that would be a breaking change (which was something else I was hoping to prevent). |
Changes
Adds type stubs to classes that clients will interact with, and
py.typed
so that type checkers know type information is available.Fixes #980 (kind of)
Per providing type annotations, there are several ways to distribute type information:
This is the second option, and does not touch any of the
.py
files. If you prefer to go with the first option (preferred, but makes changes to.py
files), please have a look at #1074 instead. That PR also includes additional details on how type information was added.Checklist
CHANGES
folder<issue_id>.<type>
(e.g.588.bugfix
)issue_id
change it to the pr id after creating the PR.feature
: Signifying a new feature..bugfix
: Signifying a bug fix..doc
: Signifying a documentation improvement..removal
: Signifying a deprecation or removal of public API..misc
: A ticket has been closed, but it is not of interest to users.Fix issue with non-ascii contents in doctest text files.