Add typing and enforce checking via tox/CI #135
Conversation
pgjones
force-pushed
the
typing
branch
2 times, most recently
from
9fb1a67
to
dfd06ba
Compare
| __all__ += _events.__all__ | ||
| __all__ += _connection.__all__ | ||
| __all__ += _state.__all__ | ||
| __all__ = ( |
There was a problem hiding this comment.
This change already is going to do a ton for discoverability 🎉 IDEs don't like dynamic __all__.
| return [(raw_name, value) for raw_name, _, value in self._full_items] | ||
|
|
||
|
|
||
| def normalize_and_validate(headers, _parsed=False): | ||
| HeaderTypes = Union[ |
There was a problem hiding this comment.
Shouldn't this be List[Tuple[Union[str, bytes], Union[str, bytes]] instead of the individual permutations?
There was a problem hiding this comment.
mypy seems to get confused by that (although technically it is more correct as it doesn't need to uniformly the same type in the tuple for all elements of the list) - I think this covers enough of the likely use cases and mypy understands it.
I'm hoping to use mypyc next to gain more performance (it was reasonably effective 2 years ago). So I'm not keen on this unless it doesn't affect mypyc. |
This uses the same mypy settings as wsproto. The Sentinel values are problematic, but I've found no good solution that also has the property that type(Sentinel) is Sentinel - so this should suffice for now. Whilst I've been lazy with the tests, I've mostly avoided type ignores in the main code. This should ensure that mypyc can be used if desired.
| SERVER = make_sentinel("SERVER") | ||
|
|
||
| class CLIENT(Sentinel, metaclass=Sentinel): | ||
| pass |
There was a problem hiding this comment.
I'm surprised that metaclass=... is needed here.
(I guess I would have assumed that inheriting from a class with a custom __new__ method was sufficient.)
Not sure this is an actionable comment, but just "this is a bit fiddly, innit?" 🙂
There was a problem hiding this comment.
Yea it is. There is some extra discussion here.
| class_ = InformationalResponse if status_code < 200 else Response | ||
| http_version = ( | ||
| b"1.1" if matches["http_version"] is None else matches["http_version"] | ||
| ) |
There was a problem hiding this comment.
This is an interesting change. Much neater/more obvious this way, now that we're not passing an open-ended **matches to the response class.
| @@ -113,7 +135,7 @@ def __call__(self, buf): | |||
| self._remaining -= len(data) | |||
| return Data(data=data) | |||
|
|
|||
| def read_eof(self): | |||
| def read_eof(self) -> NoReturn: | |||
| assert normalize_data_events( | ||
| [ | ||
| Data(data=bytearray(b"1")), | ||
| Data(data=b"2"), | ||
| Response(status_code=200, headers=[]), | ||
| Response(status_code=200, headers=[]), # type: ignore[arg-type] |
There was a problem hiding this comment.
I'm curious about these ignore cases, since it looks correct to me?
There was a problem hiding this comment.
I think mypy gets confused about the type of the empty list. There is an open bug I can't find at the moment
| Callable[[Union[InformationalResponse, Response], Writer], None], | ||
| Callable[[Request, Writer], None], | ||
| ], | ||
| ] |
There was a problem hiding this comment.
I think this typing here helps illuminate that the WRITERS usage here is a bit fiddly, with it's special case for SEND_BODY. 😀
Not actionable tho - just a passing observation.
|
|
||
| @overload | ||
| def normalize_and_validate(headers: HeaderTypes, _parsed: Literal[False]) -> Headers: | ||
| ... |
There was a problem hiding this comment.
I don't understand this overloading?
What's with the Literal[True], Literal[False], bool cases?
There was a problem hiding this comment.
It allows mypy to determine that the type of headers changes depending on _parsed being True or False. See also this
|
Thanks, I'll merge tomorrow |
|
Thanks again for the reviews and comments 😄 |
This uses the same mypy settings as wsproto.
The Sentinel values are problematic, but I've found no good solution
that also has the property that type(Sentinel) is Sentinel - so this
should suffice for now.
Whilst I've been lazy with the tests, I've mostly avoided type ignores
in the main code. This should ensure that mypyc can be used if
desired.