-
Notifications
You must be signed in to change notification settings - Fork 207
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
support retrying, if multiple exception are thrown #116
Conversation
fun <T> Flow<T>.handleErrorAndRetry( | ||
actionLabel: String, | ||
userMessageStateHolder: UserMessageStateHolder, | ||
retryAction: ((Throwable) -> Unit)? = null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a suspend lambda? This is because we might call repository.refresh(). 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course.
Suspend lambda can also be executed as a retry process.
c4f582e
userMessageStateHolder: UserMessageStateHolder, | ||
retryAction: ((Throwable) -> Unit)? = null, | ||
) { | ||
retry { throwable -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This is just my preference, but how about separating the method like this? 👀 |
@@ -196,6 +186,30 @@ fun <T1, T2, T3, T4, R> ViewModel.buildUiState( | |||
) | |||
) | |||
|
|||
fun <T> Flow<T>.handleErrorAndRetry( | |||
actionLabel: String, | |||
userMessageStateHolder: UserMessageStateHolder, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of using a context receiver to pass UserMessageStateHolder. like context(UserMessageStateHolder)
. But we can change it after merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
Since the codebase is not yet ready, let's do it after the merge. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a little suggestion, But It looks great! Thanks!
It looks good.:+1: |
…rrorAndRetry-into-handleErrorAndRetry-and-handleErrorAndRetryAction/2023-05-06
…rAndRetry-into-handleErrorAndRetry-and-handleErrorAndRetryAction/2023-05-06 Separate handleErrorAndRetry into handleErrorAndRetry and handleErrorAndRetryAction
Hi @mitohato14! Codes seem to be unformatted. Please run |
@takahirom |
The following issue: #96.
Originally crashed when multiple exceptions occurred, but changed to allow multiple retries.
Defined as an extension function at the same time.
I also thought about allowing another process to be performed instead of retrying when an exception occurs, but I decided that there would be more cases of retries, so I made the process assuming a retry.
Screen_recording_20230506_164743.mp4