fix: Retry constructors methods support None #592
Conversation
| 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?
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
| @@ -318,26 +309,42 @@ def with_predicate(self, predicate: Callable[[Exception], bool]) -> Self: | |||
| Returns: | |||
| Retry: A new retry instance with the given predicate. | |||
| """ | |||
| 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?
There was a problem hiding this comment.
It seems like predicate has always been a required variable
There was a problem hiding this comment.
Well, in _replace, we had predicate: Callable[[Exception], bool] | None = None,, right?
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
| 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.
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
There was a problem hiding this comment.
@daniel-sanche Please could you link the specific commit for the fix in #592 (comment)?
There was a problem hiding this comment.
sorry, forgot to push. The new test should be there now
Fixes #591
v2.16.0 did some refactoring of the Retry class, including adding a
_replacehelper method to simplify the constructorswith_deadline,with_timeout,with_predicate, andwith_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
Noneis 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