-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add timeout option to Client #463
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeout
type signature is complicated enough that it's probably worth defining a type alias, a la https://github.com/python/typeshed/blob/47ce3cb8dbf564130701b2344c24ff248675f6a2/stubs/requests/requests/sessions.pyi#L106. It should be a "public" type alias, unlike the typeshed (i.e. Timeout
, not _Timeout
).
1f4a4c8
to
76ab64b
Compare
bd95212
to
f7105e7
Compare
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #463 +/- ##
==========================================
- Coverage 87.51% 87.38% -0.14%
==========================================
Files 12 12
Lines 857 864 +7
==========================================
+ Hits 750 755 +5
- Misses 107 109 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
9fb8c5c
to
3b7c7c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I added a drop-dead simple smoke test just to ensure things don't break when you provide a timeout. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I pushed "approve" prematurely. One inline change, and also Client.open
needs timeout
added to its docstring.
…ht as the timeout came from the mock infrastructure, not from the request failing to serve in time
…ithout an explicit test
755dbf0
to
df8f496
Compare
Related Issue(s):
Closes #127
Description:
In order to set an explicit timeout for some requests, this PR adds a
timeout
parameter toClient.open
. This is still a work in progress, but I've been able to add a functioning timeout parameter; the remaining work includes proper test coverage (at the moment, it's not clear how to catch the error raised by this feature) and an additional pass over the API to see other places where this feature would need to be added.PR Checklist: