Skip to content

Conversation

@minglecm
Copy link

@minglecm minglecm commented Dec 5, 2017

This change adds a backward-compatible timeout option to each client method.

Without the ability to provide a timeout, requests can hang (I think up to the global max socket timeout in Python).

See: http://docs.python-requests.org/en/master/user/quickstart/#timeouts

Alternatively, a single timeout could be provided when instantiating a Client. This approach wouldn't require changes to methods other than request. However, given the wide range of methods that the client provides, this more granular timeout may be preferred.

Also, I imagine this should be a minor version bump 1.2.1 -> 1.3.0. Let me know if you'd like me to make that change in setup.py as well.

This change adds a backwards-compatible timeout option to each client method.

Without the ability to provide a timeout, requests can hang (I think up to the global max socket timeout in Python).
See: http://docs.python-requests.org/en/master/user/quickstart/#timeouts
@epels
Copy link
Contributor

epels commented Jul 3, 2018

Hi @dentafrice, thanks for opening a pull request! Being able to set a timeout for requests does indeed seem a useful enhancement.

I do think providing the timeout to the Client's constructor as an (optional) argument is the favourable option here: that would make it consistent with how this is handled in our other clients like Node and PHP. Additionally, it makes the method calls more concise. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants