add async Retry __eq__ and __hash__ & fix ExponentialWithJitterBackoff __eq__ by terencehonles · Pull Request #3668 · redis/redis-py (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Description of change
This change fixes a typo in ExponentialWithJitterBackoff's __eq__ method from #3628, and allows async Retry objects to also support __eq__ and __hash__.
Pull Request check-list
Please make sure to review and check all of these items:
- Do tests and lints pass with this change?
- Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
- Is the new or changed code fully tested?
- Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
- Is there an example added to the examples folder (if applicable)?
NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
fix ExponentialWithJitterBackoff add async Retry __eq____eq__ and __hash__ & fix ExponentialWithJitterBackoff __eq__
This comment was marked as outdated.
This change fixes a typo in ExponentialWithJitterBackoff's __eq__ method.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a typo in the ExponentialWithJitterBackoff.__eq__ method and refactors the retry system to support __eq__ and __hash__ methods for async Retry objects. The changes introduce a shared base class to eliminate code duplication between sync and async retry implementations.
- Extracts common retry functionality into an
AbstractRetrybase class with generic type support - Fixes incorrect class name check in
ExponentialWithJitterBackoff.__eq__method - Updates tests to verify both sync and async
Retryobjects support equality and hashing operations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| redis/retry.py | Refactors to introduce AbstractRetry base class and updates Retry to inherit from it |
| redis/asyncio/retry.py | Simplifies async Retry by inheriting from AbstractRetry and implementing __eq__ method |
| redis/backoff.py | Fixes typo in ExponentialWithJitterBackoff.__eq__ method class name check |
| tests/test_retry.py | Parameterizes existing tests to cover both sync and async Retry classes |
Comment on lines 80 to 81
| __init__.__doc__ = AbstractRetry.__init__.__doc__ |
|---|
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Manually copying docstrings can lead to maintenance issues. Consider using proper inheritance or decorators to share documentation, or simply let the docstring inherit naturally from the parent class.
| __init__.__doc__ = AbstractRetry.__init__.__doc__ |
|---|
Copilot uses AI. Check for mistakes.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed both of these, but I'm not sure this is actually correct. This is supposed to be the case for class docstrings, but this is a class method and testing locally (I wasn't sure of this before the review and tested myself) it did not inherit the docstring (at least when using the help method).
petyaslavova pushed a commit that referenced this pull request
…erBackoff __eq__ (#3668)
- fix ExponentialWithJitterBackoff
__eq__
This change fixes a typo in ExponentialWithJitterBackoff's __eq__ method.
create abstract retry class to share methods between retry implementations
update retry classes: remove slots & move
__eq__to concrete classesimplement init methods in retry subclasses to make the supported errors more obvious
Labels
2 participants
