feat: fully type annotate datastructures.py #1403
Conversation
|
I'm not going to do it in this PR, but it would also be good to make the arguments to |
starlette/requests.py
Outdated
| @property | ||
| def client(self) -> Address: | ||
| host, port = self.scope.get("client") or (None, None) | ||
| return Address(host=host, port=port) | ||
| return Address(host=host, port=port) # type: ignore[arg-type] |
There was a problem hiding this comment.
Hmmm... This is actually not ASGI compliant: https://asgi.readthedocs.io/en/latest/specs/www.html#http-connection-scope
client (Iterable[Unicode string, int]) – A two-item iterable of [host, port], where host is the remote host’s IPv4 or IPv6 address, and port is the remote port as an integer. Optional; if missing defaults to None.
There was a problem hiding this comment.
How is this not ASGI compliant? The NamedTuple is a tw-item utterable of [host, port]. It's the same thing we had before. In fact, I don't think we even need the # type: ignore. I just removed it in 417a3d7 and now there are 0 changes in this file.
There was a problem hiding this comment.
Because Address is not supposed to accept (None, None), client should actually be Optional[Address].
My bad anyway, I should have mentioned that it's not because of your PR... I just noticed that it's currently wrong.
There was a problem hiding this comment.
let's fix it in another PR then 😄
| self, url: str = "", scope: Scope = None, **components: typing.Any | ||
| self, | ||
| url: str = "", | ||
| scope: typing.Optional[Scope] = None, |
There was a problem hiding this comment.
| scope: typing.Optional[Scope] = None, | |
| scope: Scope = None, |
There was a problem hiding this comment.
Could you explain this change request to me please, or reference some documentation on the subject? I understand that there is both a practical and semantic difference, but as far as I understand using Scope | None is now preferred if the default value is None. For example, see https://stackoverflow.com/questions/62732402/can-i-omit-optional-if-i-set-default-to-none
| @typing.overload | ||
| def __getitem__(self, index: int) -> str: | ||
| ... # pragma: no cover | ||
|
|
||
| @typing.overload | ||
| def __getitem__(self, index: slice) -> typing.Sequence[str]: | ||
| ... # pragma: no cover |
There was a problem hiding this comment.
If this PR doesn't get merged, you can also create a separate PR for this. 👍
There was a problem hiding this comment.
Yep may have to split this up
| @@ -246,7 +270,7 @@ def __init__( | |||
| if kwargs: | |||
| value = ( | |||
| ImmutableMultiDict(value).multi_items() | |||
| + ImmutableMultiDict(kwargs).multi_items() | |||
| + ImmutableMultiDict(kwargs).multi_items() # type: ignore[operator] | |||
There was a problem hiding this comment.
Why is not a valid operation?
There was a problem hiding this comment.
because the type of value cannot be inferred from the expression value = args[0] if args or []
There was a problem hiding this comment.
this could be fixed with a bit of juggling of how this is handled, but I wanted to do this with the least amount of runtime changes necessary, so adding a type: ignore for now and maybe doing a rework in a future PR seems like the best bet
starlette/datastructures.py
Outdated
| @typing.overload | ||
| def get(self, key: _KT) -> _VT_co: | ||
| ... # pragma: no cover | ||
|
|
||
| @typing.overload | ||
| def get( | ||
| self, key: _KT, default: typing.Optional[typing.Union[_VT_co, _T]] = ... | ||
| ) -> typing.Union[_VT_co, _T]: | ||
| ... # pragma: no cover | ||
|
|
||
| def get(self, key: _KT, default: typing.Any = None) -> typing.Any: | ||
| if key in self._dict: | ||
| return self._dict[key] | ||
| return typing.cast(_VT_co, self._dict[key]) | ||
| return default |
There was a problem hiding this comment.
Not sure if I fully understand the reason of what is written here. 🤔
There was a problem hiding this comment.
This is saying:
- If you do not provide a default, the return value is going to be of type
_VT_co(so a value of the dictionary) (or an error) - If you do provide a default value, the return value is going to be either
_VT_co(if the key is found) or the type of your default value (if the key is not found)
There was a problem hiding this comment.
I removed the typing.cast by giving ImmutableMultiDict._dict a type annotation
|
Closing in favor of:
Once / if #1449 gets merged, I'll make follow ups for |
This adds annotations to
datastructes.py.Mainly this is making the mapping-like classes generic.
This means that
FormData,Headers, etc. can be properly type annotated.