-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Allow converting async block to Traits #2411
base: main
Are you sure you want to change the base?
Conversation
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.
Some comments about Task
observer(.next(element)) | ||
observer(.completed) | ||
} | ||
let task: Task<Void, Swift.Error> |
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.
Since operation isn't throwing this can be Task<Void, Never>
. (true for all four locations)
} | ||
|
||
return Disposables.create { | ||
task.cancel() |
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.
Good call to cancel the tasks, but the operation
never checks if the task was cancelled. This will only do anything if the code passed in as block
Would it make sense to add if Task.isCancelled { return }
after let element = await block()
and not send anything to observer
, as if the observable never fired anything?
Or would it make more sense to not send .next(element)
but to still send .completed
?
I left that note to show my train of thought, but now I am thinking it is best to leave this as is and not check Task.isCancelled
since it is up to the user passing in Block to decide what to do when their Task is cancelled. They still may want to return a partial result perhaps. That should be up to them.
I was thinking there should also be a way to indicate they want to return nothing when their Task gets cancelled, but I think the catch block (in the other three, not Infallible) is probably sufficient for that. If they end user cares to know their Single/Maybe/Completable threw an error because the Task was cancelled and not some other error they can check the error type.
From the documentation:
"Tasks include a shared mechanism for indicating cancellation, but not a shared implementation for how to handle cancellation. Depending on the work you’re doing in the task, the correct way to stop that work varies. Likewise, it’s the responsibility of the code running as part of the task to check for cancellation whenever stopping is appropriate. In a long-task that includes multiple pieces, you might need to check for cancellation at several points, and handle cancellation differently at each point. If you only need to throw an error to stop the work, call the Task.checkCancellation() function to check for cancellation. Other responses to cancellation include returning the work completed so far, returning an empty result, or returning nil."
1198683
to
5949cbd
Compare
This PR allows converting
async
block to RxSwift Traits -Infailable
,Single
,Maybe
,Completable
,Signal
,Driver
.