Conversation
6889fce to
412bc3d
Compare
Lost commits from 4bd79e3...0e4ae38 in the squash. This readds them.
|
|
||
|
|
||
| def request(method, url, **kwargs): | ||
| def request(method: str, url: str, **kwargs: Any) -> Response: |
There was a problem hiding this comment.
I know it would be a lot of work, but annotating the kwargs with an Unpack[KwargsDict] where KwargsDict is a typing.TypedDict would be great!
I hacked this into my own code that interacts with requests code to make my usage more safe:
- where I defined my own typed dict class https://github.com/cthoyt/pystow/blob/1ca7fa5763c6aa0d2138a164c78fd67955152ebc/src/pystow/utils/download.py#L63-L72
- where I annotate it on a function I wrote, that wraps
requsts.get: https://github.com/cthoyt/pystow/blob/1ca7fa5763c6aa0d2138a164c78fd67955152ebc/src/pystow/utils/download.py#L87 - where the kwargs get splatted into
requests.get: https://github.com/cthoyt/pystow/blob/1ca7fa5763c6aa0d2138a164c78fd67955152ebc/src/pystow/utils/download.py#L165
There was a problem hiding this comment.
also I am willing to PR this if you think it's a good idea / if you think an external contrib is better/easier than doing it yourself
There was a problem hiding this comment.
Thanks for comment, @cthoyt! This is a good callout because it is a delta from typeshed that should've been in the overview.
I played with Unpack for this but put that on pause because it got kind of messy. From what I was able to come up with we're going to need several different TypedDicts to accomplish it. Essentially one per signature, it's definitely doable, just clunky.
If you have ideas to simplify the approach, I'd love to hear more. This is something that needs an answer before we ship.
There was a problem hiding this comment.
one thing worth noting is that you can have (multiple) inheritance on typed dicts. this can make it easier to factor out similarities between typed dict definitions for several functions
| _ParamsMappingValueType: TypeAlias = ( | ||
| str | bytes | int | float | Iterable[str | bytes | int | float] | None | ||
| ) | ||
| ParamsType: TypeAlias = ( |
There was a problem hiding this comment.
it would be lovely if these type hints were available through a "public" interface, since I've often written code that wraps a requests call, where I would want to use the ParamsType to annotate my own code
I say public in quotes because I could import this, but it feels wrong
There was a problem hiding this comment.
I agree having some form of public hints for the top-level APIs is potentially warranted. I started very conservative here because often times new code goes into Requests and immediately comes someone else's critical dependency. I don't want to create a binding contract that people start surfacing in their code, and then "break" when we need to update/tweak it.
I think once we're really happy with the types, the main argument parameters could be surfaced. I've deferred that for now until we can make a more informed decision. Presumably everything calling in is already typed sufficiently.
There was a problem hiding this comment.
fair! thank you for doing this and considering carefully :)
Add inline type annotations
Important
We want feedback from people who actively maintain projects that depend on Requests or use it heavily. Please share your experience testing this against your code in the linked issue.
Comments that are clearly AI-generated will be hidden or removed. This is a library written "for Humans". The conversation is between maintainers and users, not a prompt.
Overview
This PR adds type annotations throughout Requests to codify the type contract alongside the code. This is a path for what inline typing in Requests could look like. The goals are to establish what APIs are intended to support, make more reasoned decisions about code changes, and make using Requests in a modern development environment easier.
The package passes strict type checking with pyright, with the caveat that a few decisions on internals have been deferred for discussion by the maintainers and broader community. Right now, the intended behavior for those is undefined.
Related issue: RFC: Adding inline type annotations to Requests
How to verify locally
Should report 0 errors.
typing-extensionsis needed on Python 3.10-3.12.Main design decisions
cast()overassert/isinstancefor type narrowing to avoid breaking downstream code at runtime_types.pymodule centralizing Protocols, type aliases, and TypeVarsSupportsRead/SupportsItemsas@runtime_checkableProtocols replacinghasattrchecksis_prepared()as aTypeIsguard that's a no-op at runtime (always returnsTrue)Runtime impact
A full catalog of every runtime-impacting change is available here. The behavioral changes and bug fixes are the ones worth scrutinizing. The rest is mechanical.
Open discussion points
strvsbytesURL handling: Requests has historically accepted both when Python 2.7 was primary usage. These types currently declarestronly. This needs a decision on whether to fix existing broken bytes behavior or formalize thestr-only contract.yield_requestsAPI: Needs documentation and a decision on how this should even work in practice. This API decision came through without review and I don't think there's any realistic usage outside of the library.What's NOT in this PR