Skip to content
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

Merged
merged 3 commits into from
Oct 29, 2024

Conversation

DanielNoord
Copy link
Contributor

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.

@DanielNoord DanielNoord force-pushed the type-connection-utils branch from 7e298d5 to 76105cc Compare October 23, 2024 07:23
@DanielNoord
Copy link
Contributor Author

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 :)

@DanielNoord
Copy link
Contributor Author

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.

@elprans
Copy link
Member

elprans commented Oct 23, 2024

The test failure might be a fluke?

Yeah it's a flake.

# 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]",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
protocol_factory: "Callable[[], _ProctolFactoryR]",
protocol_factory: Callable[[], _ProctolFactoryR],

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Collaborator

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)

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, do that please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@DanielNoord DanielNoord requested a review from elprans October 23, 2024 16:14
@DanielNoord
Copy link
Contributor Author

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?

Copy link
Member

@elprans elprans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@elprans elprans merged commit a273e0e into MagicStack:master Oct 29, 2024
41 checks passed
@DanielNoord DanielNoord deleted the type-connection-utils branch October 29, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants