python-trio / trio-websocket

WebSocket client and server implementation for Python Trio
MIT License
70 stars 26 forks source link

Add inline type hints #167

Closed A5rocks closed 2 years ago

A5rocks commented 2 years ago

A couple review notes:

Sorry for the large PR, but I wanted to upstream my stubs (and make sure they were right -- after all, this way you yourself can run mypy and find things!)

BTW, if I bump the python version to 3.7 in mypy's configuration, I get all these issues from the wsproto event dataclasses currently ignored:

trio_websocket/_impl.py:821: error: Argument "host" to "Request" has incompatible type "Optional[str]"; expected "str"  [arg-type]
trio_websocket/_impl.py:821: error: Argument "target" to "Request" has incompatible type "Optional[str]"; expected "str"  [arg-type]
trio_websocket/_impl.py:822: error: Argument "subprotocols" to "Request" has incompatible type "Optional[Iterable[str]]"; expected "List[str]"  [arg-type]
trio_websocket/_impl.py:823: error: Unused "type: ignore" comment
trio_websocket/_impl.py:947: error: Unused "type: ignore" comment
trio_websocket/_impl.py:1016: error: Unused "type: ignore" comment
trio_websocket/_impl.py:1031: error: Unused "type: ignore" comment
trio_websocket/_impl.py:1031: error: Argument "payload" to "Pong" has incompatible type "Optional[bytes]"; expected "bytes"  [arg-type]
trio_websocket/_impl.py:1046: error: Unused "type: ignore" comment
trio_websocket/_impl.py:1049: error: Unused "type: ignore" comment
trio_websocket/_impl.py:1079: error: Unused "type: ignore" comment
trio_websocket/_impl.py:1102: error: Unused "type: ignore" comment
trio_websocket/_impl.py:1107: error: Unused "type: ignore" comment
trio_websocket/_impl.py:1122: error: Unused "type: ignore" comment
trio_websocket/_impl.py:1256: error: Unused "type: ignore" comment
Found 15 errors in 1 file (checked 3 source files)

However, I do not want to fix them (similar to how I just added TODOs to things that were Optional and shouldn't be), as that would make the scope of the PR too broad; these could be follow-up changes!

Additionally, this might make a potential port to anyio easier since you will know what types to update!

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 233


Changes Missing Coverage Covered Lines Changed/Added Lines %
trio_websocket/_impl.py 126 133 94.74%
<!-- Total: 127 134 94.78% -->
Totals Coverage Status
Change from base Build 231: -0.6%
Covered Lines: 523
Relevant Lines: 550

💛 - Coveralls
A5rocks commented 2 years ago

I am very sorry I left this out to dry for so long. I'm here to try to push this through, although it may take a bit more work.

Here's some reasons why it's useful to have type hints in the library:

It's been a solid couple months so I will have to get re-adjusted to everything, but I'll take a look to see if something can be done about the additional dependencies and I'll bring this up to date. I'd love a re-review (and will be requesting another review after I have done everything I need to)

belm0 commented 2 years ago

trio-websocket is kind of on life support, and I'm the closest thing to a maintainer. When something does come up that needs attention, I can barely get to it. I'd prefer not to include "fighting with the type system" as more obstacles to my future self. There's are a number of areas the library could use improvement if there were time, but I wouldn't rank typing too high-- and in any case there are not enough actual users (of trio-websocket or trio) to warrant the work.

I appreciate that you've spent time on this-- but I had mentioned I'm hesitant, and hope I didn't mislead you into investing more time.

A5rocks commented 2 years ago

Ah, shame. I was planning on trying to combine wsproto with httpx's (httpcore, really) streams anyways (would support anyio + have less deps for my project that already depends on httpx), just wanted to finish this up in case people wanted it.

Thanks for the notice though, yup!

PS thanks for the library, I remember struggling with a choice and eventually choosing this one (and being happy due to its testability -- I could just chuck a memory stream at it and it would be in-memory for tests).