Add typing to two objects in connection_utils - #1198
Conversation
Sorry, something went wrong.
|
Sorry about that! I didn't have mypy and flake8 in my virtualenv so the linter tests didn't run. This is now fixed and I ran the tests locally. This should now pass CI :) |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Yeah it's a flake. |
Sorry, something went wrong.
| # 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], |
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
For compatibility you want typing.Callable, not collections.abc.Callable
Sorry, something went wrong.
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?
Sorry, something went wrong.
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)
Sorry, something went wrong.
There was a problem hiding this comment.
Also fine with me, @elprans what has your preference?
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah, do that please.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed!
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
elprans
left a comment
There was a problem hiding this comment.
Looks good, thanks!
Sorry, something went wrong.
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 the connect.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.