Skip to content
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

Add async strategies #2

Closed
wants to merge 4 commits into from
Closed

Conversation

hasier
Copy link
Owner

@hasier hasier commented Feb 2, 2024

Third and last PR after breaking down jd#433 (follows #1)

After DRYing the iter() function and making AsyncRetrying support async callbacks, add new async strategies (basically a copy-paste) and improve typing.

@hasier
Copy link
Owner Author

hasier commented Feb 2, 2024

@jd third and last PR here stacked on the second one. Please let me know if you prefer that I opened it against your main instead (which would include the commits of the previous 2 PRs too).

Comment on lines 47 to 51
class _retry_always(retry_base):
"""Retry strategy that always rejects any result."""

async def __call__(self, retry_state: "RetryCallState") -> bool: # type: ignore[override]
return True
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way for those function/classes to use the code from the sync ones? That'd avoid duplicating the logic everywhere.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Besides, I also realised most of the implementations don't even need a redefinition as they are not performing any async operations. So I have removed most of the reimplementations and I've tried to DRY some of the others that use predicates, wdyt? c619e10

This implementation still looks somewhat odd to me though, I feel that combining sync and async retries may still yield some weird results, e.g. if the sync condition goes first in an &/| condition, then it will chain the rest as sync, which might not always work. Any suggestions to work around that? I was thinking of maybe referencing the async wrapper from the base class, but it sounds messy...

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I found a solution for the issue. I have rebased this PR onto main and added the changes there, would be great to get your thoughts on that 🙂 jd#451

@hasier hasier requested a review from jd February 5, 2024 11:06
@hasier hasier marked this pull request as draft March 18, 2024 15:41
@hasier
Copy link
Owner Author

hasier commented Mar 18, 2024

Superseded by jd#451

@hasier hasier closed this Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants