Add/complete type annotations #193
Conversation
|
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:
|
| CloseReason as CloseReason, | ||
| ConnectionClosed as ConnectionClosed, |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
(the value of strict typing on test code seems relatively low)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| CloseReason as CloseReason, | ||
| ConnectionClosed as ConnectionClosed, |
There was a problem hiding this comment.
| 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)) |
There was a problem hiding this comment.
hmm, this could maybe be resolved by making WebSocketConnection generic over bytes vs str?
There was a problem hiding this comment.
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
Co-authored-by: jakkdl <[email protected]>
Co-authored-by: A5rocks <[email protected]>
In this pull request, we add/complete the type annotations for this project.