Copy whole internal state when retry_with (#233)#278
Copy whole internal state when retry_with (#233)#278mergify[bot] merged 2 commits intojd:masterfrom akapps:issues/233-retry-error-callback-no-copied
Conversation
Both `retry_error_cls` and `retry_error_callback` were missing from the copy, resulting in a copy that presents a different behavior than the original function.
|
I've run black against my changes - now the CI's green finally 😌 |
jd
left a comment
jd
left a comment
There was a problem hiding this comment.
Thank you! LGTM just one minor thing in test
| def first_set(first, second): | ||
| return second if first is _unset else first |
There was a problem hiding this comment.
There should be no need to redefine this on each call, you can just move it as a static method.
There was a problem hiding this comment.
Thank you for the review.
My thinking was that since the method is very limited in scope, making it a local function would be clearer to the reader than defining it at the module level. And copy() being called probably not that often, the performance overhead seemed acceptable.
Making a static method is too cumbersome and defeats the purpose of the function IMHO:
reraise=BaseRetrying._first_set(reraise, self.reraise),
retry_error_cls=BaseRetrying._first_set(
retry_error_cls, self.retry_error_cls
),In the end I moved it as a module-level function, if you're OK with that.
There was a problem hiding this comment.
Having it as a module-level function disallow inheritance to override the method.
I don't think it's a big deal though, so that's fine.
| self.assertEqual(h(), "Hello") | ||
|
|
||
|
|
||
| class TestRetryWith(unittest.TestCase): |
There was a problem hiding this comment.
since we're now using pytest you can get away with unittest altogether
| try: | ||
| _retryable.retry_with(stop=tenacity.stop_after_attempt(2))() | ||
| self.fail("Expected ValueError") | ||
| except Exception as exc: # noqa: B902 | ||
| self.assertIsInstance( | ||
| exc, ValueError, "Should remap to specific exception type" | ||
| ) | ||
| print(exc) |
- define `_first_set` only once - get away with unittest - use pytest.raises
2 participants
Both
retry_error_clsandretry_error_callbackwere missing fromthe copy, resulting in a copy that presents a different behavior
than the original function.
Fixes issue #233