Timeout added for opener #42
Conversation
thinkingserious
added
the
status: code review request
|
|
|
Thank you @prabhakar267! Please add this parameter to the calling functions so that it can be configurable. With Best Regards, Elmer |
thinkingserious
added
difficulty: medium
prabhakar267
force-pushed
the
timeout-fix
branch
from
35f785d to
3286220
Compare
prabhakar267
force-pushed
the
timeout-fix
branch
from
3286220 to
40a2426
Compare
|
@thinkingserious I have updated the parameter and all calling functions |
|
We have not been able to merge your Pull Request, but because you are awesome - we wanted to make sure you could still get a SendGrid Hacktoberfest shirt. Please go fill out our swag form before Nov 5th and we will send the shirt! (We know that you might have tried this before and it didn’t work, sorry about that!) You have till Nov 5th to fill out this form in order to get the Hacktoberfest shirt!Thank you for contributing during Hacktoberfest! We hope to see you in the repos soon! Just so you know, we always give away a SendGrid shirt for your first ever non-Hacktoberfest PR that gets merged. |
| @@ -143,18 +143,20 @@ def _build_client(self, name=None): | |||
| url_path=url_path, | |||
| append_slash=self.append_slash) | |||
|
|
|||
| def _make_request(self, opener, request): | |||
| def _make_request(self, opener, request, timeout=4): | |||
There was a problem hiding this comment.
This seems incomplete as I don't see a way to get a timeout value down into this call from the public api. It looks to me like the Client.getattr method's nested http_request should take a timeout parameter from the **kwargs and then pass that on to the call to self._make_request. You're hard coding a 4 second timeout here which isn't right - that should be up to the caller. The default timeout shouldn't be set to anything either to stay consistent with how every other api I've used handles timeouts.
There was a problem hiding this comment.
Great feedback @wynnw!
@prabhakar267 thoughts?
thinkingserious
added
the
type: community enhancement
thinkingserious
added
status: duplicate
Fixes #20