add new variable to default unset params #1557
Conversation
|
Nice! Two thoughts based on the original issue:
I see that beyond a couple of thumbs up, an explicit consensus was not necessarily reached in the original issue. But I agree that this is a sensible way of doing it. What do the maintainers think? |
lovelydinosaur
left a comment
There was a problem hiding this comment.
Okay, so the one remaining issue with this is the places in the Timeout class, where USE_CLIENT_DEFAULT is being used. In that context it doesn't actually make sense, because that's not what it's doing there.
For the purposes of this pull request I'd suggest we probably want to keep UnsetType/UNSET, and continue to use them for the Timeout cases without any change. (While also adding the ClientDefaultType/USE_CLIENT_DEFAULT as done here.)
| auth: typing.Union[AuthTypes, UnsetType] = UNSET, | ||
| auth: typing.Union[AuthTypes, ClientDefaultType] = USE_CLIENT_DEFAULT, | ||
| allow_redirects: bool = True, | ||
| timeout: typing.Union[TimeoutTypes, UnsetType] = UNSET, |
There was a problem hiding this comment.
So, these cases in _client.py should be:
timeout: typing.Union[TimeoutTypes, ClientDefaultType] = USE_CLIENT_DEFAULT,(But the signature of the arguments in the Timeout class should keep using UNSET)
There was a problem hiding this comment.
Actually thinking about this, it's probably also a good idea to move ClientDefaultType and USE_CLIENT_DEFAULT out of _types.py, and into this module, to keep them close to where they're actually used.
There was a problem hiding this comment.
So, these cases in
_client.pyshould be:timeout: typing.Union[TimeoutTypes, ClientDefaultType] = USE_CLIENT_DEFAULT,(But the signature of the arguments in the Timeout class should keep using
UNSET)
If I do that I need to also change the the types in the Timeout class, otherwise mypy complains.
There was a problem hiding this comment.
Okay, I'll take a look.
947cc68 to
c97e220
Compare
|
Closed via #1634 - Thanks for taking a punt at this! |
Closes #1384