Skip to content

add async Retry __eq__ and __hash__ & fix ExponentialWithJitterBackoff __eq__ #3668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jul 23, 2025

Conversation

terencehonles
Copy link
Contributor

@terencehonles terencehonles commented Jun 3, 2025

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.

@terencehonles terencehonles changed the title fix ExponentialWithJitterBackoff __eq__ add async Retry __eq__ and __hash__ & fix ExponentialWithJitterBackoff __eq__ Jun 3, 2025
@terencehonles terencehonles force-pushed the patch-1 branch 5 times, most recently from b32dc30 to 0278734 Compare June 3, 2025 15:04
@petyaslavova petyaslavova requested a review from Copilot June 9, 2025 08:12
Copilot

This comment was marked as outdated.

@terencehonles terencehonles force-pushed the patch-1 branch 2 times, most recently from 9f675cc to 1e2452b Compare July 22, 2025 14:49
Copy link
Contributor

@Copilot Copilot AI left a comment

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 AbstractRetry base class with generic type support
  • Fixes incorrect class name check in ExponentialWithJitterBackoff.__eq__ method
  • Updates tests to verify both sync and async Retry objects 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

redis/retry.py Outdated
Comment on lines 80 to 81
__init__.__doc__ = AbstractRetry.__init__.__doc__

Copy link
Preview

Copilot AI Jul 22, 2025

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.

Suggested change
__init__.__doc__ = AbstractRetry.__init__.__doc__

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

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 petyaslavova merged commit 7291deb into redis:master Jul 23, 2025
66 of 68 checks passed
@terencehonles terencehonles deleted the patch-1 branch July 23, 2025 10:59
petyaslavova pushed a commit that referenced this pull request Jul 25, 2025
…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 classes

* implement __init__ methods in retry subclasses to make the supported errors more obvious
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants