Improve Internal Logic for Network Retries#4880
Conversation
Poolitzer
left a comment
There was a problem hiding this comment.
I like the overall design, just three nitpicks
harshil21
left a comment
There was a problem hiding this comment.
I see what you meant now. However it still doesn't fix the core of the issue? If you have infinite_loop=True, and max_retries > 0, we're still incrementing retries even for successful calls.
That particular parameter combination does not happen in our library, but the function should still have correct behavior for all combinations of inputs imo.
harshil21
left a comment
There was a problem hiding this comment.
ohh nvm there is a guard against that combination of inputs. So it would work. Do tests need updating?
Except for coverage reasons, I don't see a need. this PR doesn't introduce any new functionality that would need testing. test for the existing network retry use cases are already in place :) |
|
welp, that took a bit of a detour, but ready for re-review now :) |
harshil21
left a comment
There was a problem hiding this comment.
looks good, just one question 👍🏽
My take on closing #4871 :) Maybe the diff explains my intentions better than my words :D