-
Notifications
You must be signed in to change notification settings - Fork 403
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 typing to two objects in connection_utils
#1198
Conversation
7e298d5
to
76105cc
Compare
Sorry about that! I didn't have |
The test failure might be a fluke? I don't really understand why the changes in this PR would only break Windows but not Linux or on other versions of Python for Windows. |
Yeah it's a flake. |
asyncpg/connect_utils.py
Outdated
# TODO: The return type is a specific combination of subclasses of | ||
# asyncio.protocols.Protocol that we can't express. For now, having the | ||
# return type be dependent on signature of the factory is an improvement | ||
protocol_factory: "Callable[[], _ProctolFactoryR]", |
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.
protocol_factory: "Callable[[], _ProctolFactoryR]", | |
protocol_factory: Callable[[], _ProctolFactoryR], |
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.
That's not possible sadly. Callable
isn't subscribtable at runtime like this on Python 3.8. See earlier CI runs in which CI complained about this.
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.
For compatibility you want typing.Callable
, not collections.abc.Callable
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.
In later versions collections
is preferred though. Isn't the use of stringified annotations the way to get compatibility?
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.
If you add from __future__ import annotations
to the top of the file, you won't have to use strings in the annotation (and importing from collections.abc
will also work)
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.
Also fine with me, @elprans what has your preference?
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.
Yeah, do that please.
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.
Fixed!
Anything else missing from this PR that I can help with? Do you want me to squash the commits or are they good like this? |
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, thanks!
I noticed that it would be very useful to have a fully typed
connection_utils
as it is the core for a lot of other modules that interact with theconnect.Connection
.This just adds some basic parameter annotation that is pretty straightforward. I have also decided to be pragmatic and add a
TODO
for stuff that is just too hard right now. We can always revisit cases like this if we ever do want to enable a type checker. In the meantime, having callers of the internal function_create_ssl_connection
get the correct type already helps with typing the public API correctly.