fix: Retry constructors methods support None - #592
Conversation
Sorry, something went wrong.
vchudnov-g
left a comment
There was a problem hiding this comment.
Some questions and test suggestions, but the core functionality LGTM.
Sorry, something went wrong.
| if not use_deadline | ||
| else retry_.with_deadline(value) |
There was a problem hiding this comment.
Under the hood, with_deadline is just calling with_timeout, so these two branches wind up testing the same code, module the trivial wrapping function. Right?
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah, I believe so. I think the intention is to make sure they stay in-line through any potential future refactors
Sorry, something went wrong.
| """ | ||
| return self._replace(predicate=predicate) | ||
| return type(self)( | ||
| predicate=predicate, |
There was a problem hiding this comment.
Are we allowing predicate to be None here? If so, we would need to tweak the type annotation, and to add a test for this edge case? (The test might be starting with something with a non-trivial predicate, and then verifying that calling .with_predicate(None) sets it to None?
Sorry, something went wrong.
There was a problem hiding this comment.
It seems like predicate has always been a required variable
Sorry, something went wrong.
There was a problem hiding this comment.
Well, in _replace, we had predicate: Callable[[Exception], bool] | None = None,, right?
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, I missed this comment
In the _replace helper, None represented a variable that isn't replaced (that is, it used the same value as the original Retry object).
But this doesn't work if users are passing in None for a with_timeout and expecting it to disable timeouts, which wasn't supported by the type annotations, but was used in practice. Which was the motivation for this change
Sorry, something went wrong.
| return self._replace(initial=initial, maximum=maximum, multiplier=multiplier) | ||
| return type(self)( | ||
| predicate=self._predicate, | ||
| initial=initial if initial is not None else self._initial, |
There was a problem hiding this comment.
If these values are not None but still falsy, we still want to use them? If so, we should capture this in a test since this differs from the previous behavior.
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah it differs from the previous behaviour, but it is in line with the pre-2.16.0 behaviour
Good point on the test. I had added one for timeout, but not for this one. Fixed
Sorry, something went wrong.
There was a problem hiding this comment.
@daniel-sanche Please could you link the specific commit for the fix in #592 (comment)?
Sorry, something went wrong.
There was a problem hiding this comment.
sorry, forgot to push. The new test should be there now
Sorry, something went wrong.
Fixes #591
v2.16.0 did some refactoring of the Retry class, including adding a _replace helper method to simplify the constructors with_deadline, with_timeout, with_predicate, and with_delay. But it was noticed there is a bug in the implementation, where falsy values are ignored, with the existing value silently used instead.
Note that None is technically not supported by the type annotations, and 0 is typically not a valid value for these fields, which is likely why there was a gap in the tests around this. But the annotations were added recently, and existing code use pass None in for a timeouts, and None is supported by the internal retry code down the stack. For this reason, I changed the type annotation for the timeout field to support None as part of this fix, and added tests to ensure that it will be supported in the future