Add/complete type annotations by CoolCat467 · Pull Request #193 · python-trio/trio-websocket

CoolCat467 · 2025-01-13T04:26:24Z

In this pull request, we add/complete the type annotations for this project.

A5rocks · 2025-01-13T09:29:18Z

I'd like @belm0 to comment on whether this is fine now. As of two years ago they preferred not adding type hints without material gain (#167 (comment)) but things have changed since:

more changes were necessary due to strict exception groups
it's been move into the trio org (I don't think it was here back then? Maybe I'm misremembering)
type hints have gotten another 2 years to become more standard and now there's nicer ways to write some stuff (like generic builtins and | for union)

@belm0 while I have you here, mind rereviewing #186?

belm0 · 2025-01-14T03:55:55Z

trio_websocket/__init__.py

+    CloseReason as CloseReason,
+    ConnectionClosed as ConnectionClosed,


why is this needed?

This is the syntax to re-export something. The alternatives are:

from a import b as b

or

from a import b __all__ = ('b',)

(I think?)

I understand this looks pretty weird, but this is done to inform type checkers that we are intending on publicly re-exporting these imports. Basically the problem is rooted in that other projects aren't as careful about what they export as Trio and this project, so typecheckers are set up to think that anything imported in your module is not intended to be used by people importing your module. Ie users using sys import from your module instead of importing sys themselves. Issue is though, how to tell type checkers that we are indeed intending on making this import public to consumers? Solution is this, import x as x, which while it's a bit ugly, it's better than making comments or something weight bearing.

Without this, anyone using this module will encounter type issues on their end about these attributes not being publicly re-exported. There are several places in the test code that need type ignores because the tests access trio.__version__, which hasn't been marked as publicly re-exported.

see https://typing.readthedocs.io/en/latest/spec/distributing.html#library-interface-public-and-private-symbols for docs on it

mypy.ini

trio_websocket/_impl.py

belm0 · 2025-01-14T04:09:02Z

tests/test_connection.py

(the value of strict typing on test code seems relatively low)

I agree to some extent, and at the time I did it purely to stop CI from raising errors, but I think it can still be useful for reference purposes in the event there is a CI failure and someone is unsure exactly why. Type annotations are not purely for type checking, they can also be used as a part of better documenting codebases and helping developers better understand what is going on.

Can't we remove the tests from the files mypy checks?

I haven't looked at this PR yet but looking at the other comments left I'm not sure tests are worthwhile to typecheck: things like making a fake SocketListener are obviously impossible to make typecheck. Notably, there's nearly as much of a diff in the tests as in the implementation and it drowns out things.

But also I haven't looked at the tests so it might be that they caught usage issues with types while you were adding them? I assume we could just look at the actual module code itself.

I don't think strict typing is a good value for tests. It's a burden on top of something that's already hard to do, and there is no argument to be made about being an aid to API users, because it's test code.

I'm neutral on adding typing to the websocket API and impl, but -1 on adding typing to tests.

I'm +1 on adding typing to tests, especially for libraries. Even if you need a bunch of type: ignores or casts they can catch issues with type hints that would otherwise go unnoticed until an end user tries to type their code using your library and hitting arcane errors.
In trio we definitely found a bunch of this.

I don't think you should have strong expectations that the tests shouldn't contain casts or type: ignores though. If your test is doing weird shit that we don't expect users to do, then ignore away.

But e.g. it may be a sign that the types should be loosened if the library is not accepting MemoryStreams - since end users will themselves be doing tests with MemoryStreams+trio-websocket+their stuff, and it all in fact works with MemoryStreams

jakkdl

Awesome job! Mostly just a bunch of nitpicks

autobahn/client.py

mypy.ini

setup.py

jakkdl · 2025-01-15T11:47:07Z

trio_websocket/__init__.py

+    CloseReason as CloseReason,
+    ConnectionClosed as ConnectionClosed,


see https://typing.readthedocs.io/en/latest/spec/distributing.html#library-interface-public-and-private-symbols for docs on it

trio_websocket/_impl.py

jakkdl · 2025-01-15T11:58:21Z

trio_websocket/_impl.py

+            msg: str | bytes
+            # Type checker does not understand `_message_parts`
+            if isinstance(event, BytesMessage):
+                msg = b''.join(cast("list[bytes]", self._message_parts))
+            else:
+                msg = ''.join(cast("list[str]", self._message_parts))


hmm, this could maybe be resolved by making WebSocketConnection generic over bytes vs str?

Would be a pretty good idea if not for the issue that at a protocol level messages can be str | bytes and client doesn't get to choose what they receive

trio_websocket/_impl.py

Co-authored-by: jakkdl <h6+github@pm.me>

Co-authored-by: A5rocks <git@helvetica.moe>

jakkdl

Think this can be merged now

belm0 · 2025-02-15T16:31:28Z

you did a merge commit rather than squash & merge... 😢

Co-authored-by: jakkdl <h6+github@pm.me> Co-authored-by: A5rocks <git@helvetica.moe>

Add/complete type annotations

00fc838

CoolCat467 added the enhancement New feature or request label Jan 13, 2025

CoolCat467 requested review from A5rocks and jakkdl January 13, 2025 04:26

CoolCat467 added 4 commits January 12, 2025 22:36

Fix line-too-long errors

c49321f

Fix remaining CI issues

45c06ad

Use strings for cast values

a2d83f5

Missed another type quote

335dab2

belm0 reviewed Jan 14, 2025

View reviewed changes

CoolCat467 added 3 commits January 13, 2025 23:00

Remove redundant command line flags

fd09470

Remove now un-needed annotation for socket listeners

b5233c8

Re-add type ignore because of old trio version support

ce73cb1

CoolCat467 mentioned this pull request Jan 14, 2025

Publically re-export __version__ python-trio/trio#3186

Merged

jakkdl requested changes Jan 15, 2025

View reviewed changes

CoolCat467 and others added 3 commits January 15, 2025 12:09

Suggestions from code review

e98f233

Co-authored-by: jakkdl <h6+github@pm.me>

Fix lint issues

96e437f

Avoid a few type errors with fake socket listener

35a6235

Co-authored-by: A5rocks <git@helvetica.moe>

jakkdl approved these changes Jan 30, 2025

View reviewed changes

CoolCat467 merged commit f5b2014 into python-trio:master Jan 30, 2025
9 checks passed

CoolCat467 deleted the type-annotations branch January 30, 2025 15:14

belm0 pushed a commit that referenced this pull request Feb 16, 2025

Add/complete type annotations (#193)

49b93c1

Co-authored-by: jakkdl <h6+github@pm.me> Co-authored-by: A5rocks <git@helvetica.moe>

belm0 mentioned this pull request Feb 25, 2025

Use correct port for wss URLs when ssl_context is None #197

Merged

		CloseReason as CloseReason,
		ConnectionClosed as ConnectionClosed,

Uh oh!

Conversation

CoolCat467 commented Jan 13, 2025

Uh oh!

A5rocks commented Jan 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A5rocks Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

belm0 Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakkdl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoolCat467 Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jakkdl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

belm0 commented Feb 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

A5rocks Jan 16, 2025 •

edited

Loading

belm0 Jan 16, 2025 •

edited

Loading

CoolCat467 Jan 15, 2025 •

edited

Loading