Add type hints (part 1) #72
Conversation
eltbus
force-pushed
the
refactor/add-type-hints-part-1
branch
from
c23b70b
to
3b0eef8
Compare
pyproject.toml
Outdated
| exclude = [ | ||
| "**/__pycache__", | ||
| ] |
| exclude = [ | ||
| "**/__pycache__", | ||
| ] | ||
| reportUndefinedVariable = false # TODO: required because pyright does not work with __future__.annotations |
There was a problem hiding this comment.
It does work... What do you mean by this?
There was a problem hiding this comment.
Hi @Kludex!
What I mean by this is that Pyright will raise "false positive" undefinedVariable warnings.
➜ ~ pyright --version
pyright 1.1.349
➜ ~ cat kludex.py
from __future__ import annotations
def foo(s: str) -> List[str]: # Gonna get a Pyright complain here
return s.split()
➜ ~ pyright kludex.py
/Users/eltbus/kludex.py
/Users/eltbus/kludex.py:3:20 - error: "List" is not defined (reportUndefinedVariable)
1 error, 0 warnings, 0 informations
There was a problem hiding this comment.
Pyright is right there... You need to use list instead of List. 🤔
List should be imported if you want to use it anyway.
There was a problem hiding this comment.
Oh, I believed this would not work for < Python3.9, but it indeed works.
Nonetheless pyright will also complain for Optional, Union, Callable, and others unless explicitly imported from typing.
There was a problem hiding this comment.
I mean, that's a python thing, not a Pyright... If the class is not present, and is not built-in, it needs to be imported.
Instead of Optional[T], use T | None; instead of Union[T, K], use T | K; Callable just needs to be imported.
There was a problem hiding this comment.
Isn't that Python3.10 syntax?
There was a problem hiding this comment.
If you use __future__.annotations, what is on the type annotations doesn't matter for runtime.
| @@ -33,10 +35,16 @@ class Base64Decoder: | |||
| :param underlying: the underlying object to pass writes to | |||
| """ | |||
|
|
|||
| def __init__(self, underlying): | |||
| def __init__(self, underlying: IOBase): | |||
There was a problem hiding this comment.
It is a stream or file-like object with a read/write interface. From Base64Decoder's docstring:
"""
...
from multipart.decoders import Base64Decoder
fd = open("notb64.txt", "wb")
decoder = Base64Decoder(fd)
...
I would appreciate more feedback on this from you though.
multipart/multipart.py
Outdated
| @overload | ||
| def parse_options_header(value: str) -> Tuple[bytes, Dict[bytes, bytes]]: ... | ||
|
|
||
| @overload | ||
| def parse_options_header(value: bytes) -> Tuple[bytes, Dict[bytes, bytes]]: ... | ||
|
|
||
| def parse_options_header(value): |
There was a problem hiding this comment.
Why not just this?
| @overload | |
| def parse_options_header(value: str) -> Tuple[bytes, Dict[bytes, bytes]]: ... | |
| @overload | |
| def parse_options_header(value: bytes) -> Tuple[bytes, Dict[bytes, bytes]]: ... | |
| def parse_options_header(value): | |
| def parse_options_header(value: Union[str, bytes]) -> Tuple[bytes, Dict[bytes, bytes]]: |
| class Cache(Generic[T]): | ||
| __slots__ = ('_value', '_is_set') | ||
|
|
||
| def __init__(self): | ||
| self._value: Optional[T] = None | ||
| self._is_set: bool = False | ||
|
|
||
| @property | ||
| def value(self) -> Optional[T]: | ||
| if self.is_set: | ||
| return self._value | ||
| else: | ||
| raise ValueError("Value not yet set") | ||
|
|
||
| @value.setter | ||
| def value(self, v: T): | ||
| self._value = v | ||
| self._is_set = True | ||
|
|
||
| def clear(self): | ||
| """Reset the cache""" | ||
| self._value = None | ||
| self._is_set = False | ||
|
|
||
| def is_set(self) -> bool: | ||
| """Check if value has been set""" | ||
| return self._is_set |
There was a problem hiding this comment.
Can we wait for this kind of change till we have a better test coverage?
There was a problem hiding this comment.
Your call. I believe I added this to fix type-hinting for Field.__repr__ only, so it is not important.
There was a problem hiding this comment.
I'd prefer for this PR to not have runtime changes, unless really needed.
|
I'll manage this. Sorry for making you lose time. |
Partial type hinting + refactor of the cache implementation in Field