Add inline type hints

[A5rocks]

A couple review notes:

• I removed python 3.5 support as a) that's very very deprecated, and b) variable type hints would make this much worse.
• I recommend you try to check this against a codebase that uses this, as I may have forgotten to make a type as wide as the docs promise!

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]

Pull Request Test Coverage Report for Build 233

• 127 of 134 (94.78%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.6%) to 95.091%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
trio_websocket/_impl.py	126	133	94.74%

Totals
Change from base Build 231:	-0.6%
Covered Lines:	523
Relevant Lines:	550

---

💛 - Coveralls

[belm0]

Thank you for the proposal-- I'm a little hesitant to go full type-hints with trio-websocket.

There are benefits, but they doesn't come for free-- at the cost of more verbose code, more complexity, and more dependencies.

Things like the tedious re-exporting, or having to disable useful pylint checkers, are example downsides. Also, it can be corrected, but it seems unnecessary to type hint variables and args that can be inferred by initialization (max_message_size: int = MAX_MESSAGE_SIZE, etc.).

May I ask if you're actively using trio-websocket in a project? Also, did this exercise uncover any bugs in the implementation?

(This comment is overall for the PR, but I'll start the thread here.)

[A5rocks]

May I ask if you're actively using trio-websocket in a project?

Yep! Though I have just gotten by by including those stubs in a folder.

Project is open-source actually: https://github.com/A5rocks/bloom

Also, did this exercise uncover any bugs in the implementation?

As mentioned, I started with a base set of stubs so I didn't find any bugs in my implementation.

There are benefits, but they doesn't come for free-- at the cost of more verbose code, more complexity, and more dependencies.

Yep, although I should be able to just extract the stubs out into .pyi files (I haven't played with mypy's stubgen and stubtest yet), if the downsides are too much.

[belm0]

I don't know if external pyi files improves much. stubgen would just provide a starting point, and would have to be manually edited and kept up to date with the source. And it doesn't address some of the complexity issues (typing can get quite complex, such as nested types, async generators, etc.).

[A5rocks]

Hey, sorry for taking a while to get back, wanted to do this when I felt I had enough time to actually write a decent response.

Since you seem to already know many of the downsides, let me try my hand at listing the upsides of having type hints in the code itself:

1. While not the greatest guarantees, making sure to satisfy type hints can help reduce races (why? because type systems want the state to always be there). This doesn't help too much, but I felt it was worth mentioning.
2. More up to date than documentation.

One example of this that comes to mind is CloseReason.reason which was documented as a string but was actually optional.

3. Easier to check that they are right than 3rd party stubs

Here's a relevant part of a diff between stubs extracted from trio-websocket now vs the ones I had included in my project mentioned earlier (left is new stubs, right is old stubs)

90c71
<     def handshake_headers(self) -> typing.Tuple[typing.Tuple[bytes, bytes], ...]: ...
---
>     def handshake_headers(self) -> Tuple[Tuple[str, str], ...]: ...

Also, I had CloseReason.reason as an non-optional string.

4. Should help other projects.

I'll be honest, I had trouble finding a project using code search that used both mypy and trio-websocket.

However, if I only try to get trio-websocket, then I find some stuff like https://github.com/AcckiyGerman/python_trio_websockets_stress_test

Here's what I get as mypy output:

➜  python_trio_websockets_stress_test git:(master) mypy --check-untyped-defs server.py stress-test.py          (env: tmp-a92fbb44261cfc4) 
Success: no issues found in 2 source files

which isn't great yeah.

If I annotate the function arguments (oops, though check-untyped-defs should have helped a bit maybe)... then it still doesn't find anything. Perhaps this project was too trivial to check against.

So I invite you to try yourself, but I believe this holds true? (If you want to try against some project of yours that has more complex usage then that would be nice)

---

I'll definitely rebase soon + try to clean up unnecessary type hints, of course.

[belm0]

I've just merged an existing PR that drops 3.5.

[A5rocks]

Ah, cool. I won't be able to rebase right now (life catching up), but I will soon-ish!

[A5rocks]

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:

• Detects (some) bugs. This is always nice. IMO users of the library will benefit moreso than someone who knows the library inside and out, but whatever.
• Keeps documented types up to date (I would be OK sending a PR that updates the documentation to match the types I have here)
• The locality makes it easier to check the types against the code (I realize stubcheck exists in mypy but I'd rather not go there)
• Autocomplete in IDEs is always nice.

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]

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]

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