Add typing to two objects in connection_utils
#1198
Conversation
DanielNoord
force-pushed
the
type-connection-utils
branch
from
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.
| protocol_factory: "Callable[[], _ProctolFactoryR]", | |
| protocol_factory: Callable[[], _ProctolFactoryR], |
There was a problem hiding this comment.
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.
For compatibility you want typing.Callable, not collections.abc.Callable
There was a problem hiding this comment.
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.
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.
Also fine with me, @elprans what has your preference?
|
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? |
I noticed that it would be very useful to have a fully typed
connection_utilsas 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
TODOfor 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_connectionget the correct type already helps with typing the public API correctly.