Additional typing hints for requests #7696
Conversation
milanboers
commented
Apr 26, 2022
milanboers
commented
- Fixes issue where positional parameters were accepted for keyword-only arguments.
- Complete typing hints for send, request and HTTP-method functions
|
Thanks @AlexWaygood . Was struggling with this mypy bug python/mypy#11098 but seems that you found a workaround 😁 |
No worries! I figured it was easier just to push to your branch in this case, given that the errors were so minor 🙂 Thanks for the PR, it looks extremely well put together for a first-time contribution! I'm taking a look now. |
stubs/requests/requests/sessions.pyi
Outdated
| _Auth: TypeAlias = Union[tuple[str, str], _auth.AuthBase, Callable[[PreparedRequest], PreparedRequest]] | ||
| _Cert: TypeAlias = Union[str, tuple[str, str]] | ||
| _Data: TypeAlias = str | bytes | Mapping[str, Any] | Iterable[tuple[str, str | None]] | IO[Any] | ||
| _Files: TypeAlias = ( | ||
| MutableMapping[str, IO[Any]] | ||
| | MutableMapping[str, tuple[str, IO[Any]]] | ||
| | MutableMapping[str, tuple[str, IO[Any], str]] | ||
| | MutableMapping[str, tuple[str, IO[Any], str, _TextMapping]] | ||
| ) |
There was a problem hiding this comment.
Would you mind possibly providing some explanation as to how you figured out some of these more complex types? It might be quicker than us trawling through the requests codebase and repeating the steps you've already done :)
Also, just to check, were you looking at the requests main branch when you prepared this PR, or the 2.27 tag? If the former, note that the main branch is for requests 3.0, which hasn't yet been released -- these stubs are for requests 2.27. Some of the types look like they might be slightly different between 2.27 and main.
There was a problem hiding this comment.
Most of these types were already there, defined on Session.request:
I only extracted them to a TypeAlias to reuse in get, post, put etc. (these all internally call
send) and made some minor changes.
I didn't check them thoroughly, but it looks like they're mostly correct, especially if you follow request's documentation.
Request's code seems to accept more than the documentation though, not sure how far we want to go in accepting arguments that would technically work, but are not documented.
For example:
- In
files, the documentation specifically mentions to use 2/3/4-tuples, but passing lists also works. allow_redirectsis documented as optional andTrueby default. Made the typeboolnow, but I think the code will acceptNone, and make it behave likeFalse.
There was a problem hiding this comment.
Okay great, thanks, that's all really helpful!
There was a problem hiding this comment.
The diff for this PR is quite big, so I support keeping the annotations as they are for now and factoring them out into TypeAliases, as you've done in this PR. We can always improve the types in a separate PR later on 👍
There was a problem hiding this comment.
For boolean parameters specifically, it's almost always the case that any object that supports __bool__ (in other words, almost any object anywhere) can be passed and works fine at runtime. But that's not very useful for typing, so we do want to annotate such parameters as bool unless there's some specific reason (e.g., documentation) to allow a broader type.
stubs/requests/requests/api.pyi
Outdated
| method: str | bytes, | ||
| method: str, |
There was a problem hiding this comment.
Why change this? Seems like bytes works fine, even if it is a little weird:
>>> import requests
>>> requests.request(b'GET', 'https://www.bbc.com/news')
<Response [200]>There was a problem hiding this comment.
Ah, you're right! Changed it back.
The implementation for url has an explicit check+conversion for bytes, which is not there for method. They also call .upper() on it, but apparently that also works for bytes :-)
|
|
||
| _Data: TypeAlias = str | bytes | Mapping[str, Any] | Iterable[tuple[str, str | None]] | IO[Any] | ||
| _Auth: TypeAlias = Union[tuple[str, str], _auth.AuthBase, Callable[[PreparedRequest], PreparedRequest]] | ||
| _Cert: TypeAlias = Union[str, tuple[str, str]] |
There was a problem hiding this comment.
Should this accept bytes? adapters.pyi currently does this:
Looking at sessions.py, it seems to just pass everything to the adapter, so if adapters work with bytes, then we should allow bytes here too. In adapters.py, the certs are validated with os.path.exists, which works with bytes and strings.
In general, adapters.pyi seems to be a bit out of sync with api.pyi and sessions.pyi, but I don't think fixing it belongs to this PR.
There was a problem hiding this comment.
Looks like it would work for requests, but in the end it ends up at urllib3's HTTPSConnectionPool. Not sure if that accepts & keeps accepting bytes. Their current version doesn't have type stubs, but their upcoming major version requires str.
I can make a follow-up PR later to work away some of the inconsistencies and take a look at models.
