Open tschaume opened 3 years ago
@analogue any plans to consider this PR for merging?
@macisamuele I noticed you reviewed another recent PR :) Any chance you could look over this one, too? Do you also have merge and PyPI release permissions for this repo?
As I'm no longer a Yelp employee and as I'm getting less exposure to the library usage I would defer the decision to @analogue (or someone currently within the Yelp org).
My 2 cents around the PR
None
to TIMEOUT_MAX
) does represent a backward incompatible change and as such it would require a major version bump.bravado
is actually the best place to fix it (seems an issue within the fido
handling of "infinite" timeout).None
to TIMEOUT_MAX
do we still want to accept None
as a possible timeout value? If not then we would need to fix the typing annotations associated to timeout
.My recommendation in this case would be to expand more on
Hi, just to add that I've also seen reports of multiple Windows users encountering this bug who are using our bravado-powered API client (specifically, they encounter a OverflowError: timeout value is too large
). A merge or a fix would be much appreciated!
Using
timeout=None
as default causes the OverflowError to be triggered in Windows. This PR fixes #470 by usingthreading.TIMEOUT_MAX
(docs) as default. It also relates to the discussion in Yelp/fido#52.