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

feat: add stacktrace parameter on retry #211

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

drown0315
Copy link

Add stacktrace parameter on retryIf and onRetry function for getting additional information

@@ -177,8 +177,8 @@ Future<T> retry<T>(
double randomizationFactor = 0.25,
Duration maxDelay = const Duration(seconds: 30),
int maxAttempts = 8,
FutureOr<bool> Function(Exception)? retryIf,
FutureOr<void> Function(Exception)? onRetry,
FutureOr<bool> Function(Exception, StackTrace)? retryIf,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think users should be allowed to condition the retry on the stacktrace.

if you're doing this something is very wrong. It's probably better to fix the thing that is wrong.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, retryIf should not carry the stackTrace parameter.
Updated.

FutureOr<bool> Function(Exception)? retryIf,
FutureOr<void> Function(Exception)? onRetry,
FutureOr<bool> Function(Exception, StackTrace)? retryIf,
FutureOr<void> Function(Exception, StackTrace)? onRetry,
Copy link
Member

Choose a reason for hiding this comment

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

This could be useful, but is it worth doing a breaking change over this?

Maybe we can make a new optional parameter instead of onRetry or to complement onRetry. Like logRetry?

Copy link
Author

Choose a reason for hiding this comment

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

Updated.

@drown0315 drown0315 force-pushed the feature/retry_add_stacktrace branch 3 times, most recently from 32672d6 to 120c252 Compare July 23, 2023 06:48
@drown0315 drown0315 requested a review from jonasfj July 24, 2023 10:49
Future<T> retry<T>(
FutureOr<T> Function() fn, {
FutureOr<bool> Function(Exception)? retryIf,
FutureOr<void> Function(Exception)? onRetry,
Function? onRetry,
Copy link
Member

Choose a reason for hiding this comment

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

With this we loose typing.

It's a valid way to solve this, but how about doing:

FutureOr<void> Function(Exception)? onRetry,
FutureOr<void> Function(Exception, StackTrace)? logRetry,

Is there any reason we can't have both?

We could even do:

@Deprecated('Use `logRetry` instead of `onRetry`')
FutureOr<void> Function(Exception)? onRetry,
FutureOr<void> Function(Exception, StackTrace)? logRetry,

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there is an annotation or comment that'll allow us to hide the parameter from auto-completion :D

Copy link
Author

@drown0315 drown0315 Jul 25, 2023

Choose a reason for hiding this comment

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

@Deprecated('Use `onRetryFailure` instead of `onRetry`')
FutureOr<void> Function(Exception)? onRetry,
FutureOr<void> Function(Exception, StackTrace)? onRetryFailure,

Thank for review.

Updated.
Do you think 'onRetryFailure' would be a good name for this?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, isn't logging the only legitimate use-case for onRetry, hence, logRetry might be a good name, or retryLog or???

We could also all it whenRetry.

Copy link
Member

@jonasfj jonasfj Jul 26, 2023

Choose a reason for hiding this comment

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

Hmm, we'll have a hard time breaking this API in the future. So I propose not making mistakes.

onRetryFailure is a bit long, and also implies that it should be called when retrying fails.

We could also call it onError, but that name is used else where to create an alternative value when an error happens.

I'm open to other ideas than logRetry, but let's try to keep it short and elegant :D

Copy link
Author

@drown0315 drown0315 Jul 26, 2023

Choose a reason for hiding this comment

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

I think of these names

  • (i) onFailure. regardless of whether Exception satisfies retryIf, it will be called.
  • (ii) onError. any error on retry, it will be called.

Which one do you think is better?

@drown0315 drown0315 force-pushed the feature/retry_add_stacktrace branch from 120c252 to 0c0e20c Compare July 26, 2023 03:38
@drown0315 drown0315 requested a review from jonasfj July 26, 2023 03:45
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