Skip to content
This repository was archived by the owner on Aug 28, 2025. It is now read-only.

add timeout support to upwork.client#5

Merged
mnovozhylov merged 2 commits intoupwork:masterfrom
michael-yin:master
Sep 9, 2015
Merged

add timeout support to upwork.client#5
mnovozhylov merged 2 commits intoupwork:masterfrom
michael-yin:master

Conversation

@michael-yin
Copy link
Contributor

When I am using python upwork api, I found sometime the program was blocked because of the unstable network. So I think it really make sense to make the client can support timeout setting.

@mnovozhylov
Copy link
Contributor

@michael-yin , thanks for the patch!
it looks good in common, except the default timeout value that shouldn't be so high - the patch will be accepted with the default value set to '3' secs (it should be enough not to kill someone's application)

@michael-yin
Copy link
Contributor Author

Based on my experience, the default timeout for software which have socket connection is usually 30-60 secs by default.

Many developers do not set timeout when using this api, so my suggestion is we can set the default to a high value but not a low value.

If a programmer met timeout exception, he will first check the network but not change the timeout value to a higher value. Which might make them confused.

@mnovozhylov
Copy link
Contributor

Having a high value for timeout option is a really poor practise - if smth goes wrong, such requests will hang out and may overload server's pool

We can accept the patch only with small value - in case someone need a high one, he will be able to setup it in his own code

@michael-yin
Copy link
Contributor Author

Ok, I have modified the code as you asked, I think it can be merged now.
Thanks.

mnovozhylov added a commit that referenced this pull request Sep 9, 2015
add timeout support to upwork.client
@mnovozhylov mnovozhylov merged commit 50a3832 into upwork:master Sep 9, 2015
@mnovozhylov
Copy link
Contributor

@michael-yin , good job! 👍

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants