add evalOn(executor) #3637
add evalOn(executor) #3637
Conversation
There was a problem hiding this comment.
Thanks!
There is one thing I didn't think of, that we should add a test for: Scala has an ExecutionContextExecutor type that is both an ExecutionContext and an Executor. What happens if someone passes that to evalOn, how does it disambiguate?
https://www.scala-lang.org/api/2.13.10/scala/concurrent/ExecutionContextExecutor.html
| def evalOn[A](fa: IO[A], executor: Executor): IO[A] = | ||
| IO.executionContext | ||
| .flatMap(ec => IO(ExecutionContext.fromExecutor(executor, ec.reportFailure))) | ||
| .flatMap(ec => fa.evalOn(ec)) |
There was a problem hiding this comment.
Nice! This looks exactly like what I imagined. We can make this the default implementation in Async.
I have one more suggestion: we can pattern match and check if the Executor is already an ExecutionContext, and skip the conversion in that case.
We do something similar here.
cats-effect/kernel/shared/src/main/scala/cats/effect/kernel/Async.scala
Lines 200 to 206 in 7e24bb9
There was a problem hiding this comment.
Hope, I got you right. Please, check new commit when you can.
There was a problem hiding this comment.
regarding tests, i have found tests only on laws for Async's methods.
But from the comment you wrote me, i shall rather check whether the executionContext was executed through provided executor... correct me if I am wrong.
Thank you in advance!
|
@armanbilge is there everything alright? or something's missing? |
There was a problem hiding this comment.
I think what's mainly missing is the test suggested in this comment.
(@armanbilge I've figured out what's happening with the "green" CI: in the PR list, and besides the commit there is a green checkmark. However, if I log in, I see the yellow warning at the bottom. This is probably normal, just a bit strange...)
| def evalOn(executor: Executor)(implicit F: Async[F]): F[A] = | ||
| Async[F].evalOnExecutor(wrapped, executor) |
There was a problem hiding this comment.
| def evalOn(executor: Executor)(implicit F: Async[F]): F[A] = | |
| Async[F].evalOnExecutor(wrapped, executor) | |
| def evalOnExecutor(executor: Executor)(implicit F: Async[F]): F[A] = | |
| Async[F].evalOnExecutor(wrapped, executor) |
Would you mind also adding this method directly on IO? Thanks!
There was a problem hiding this comment.
@armanbilge hello! Could you please check?)
There was a problem hiding this comment.
I think this is good to go. @armanbilge any thoughts?
Thank you for tackling this!
| */ | ||
| def evalOnExecutor(executor: Executor): IO[A] = | ||
| executionContext | ||
| .flatMap(ec => IO(ExecutionContext.fromExecutor(executor, ec.reportFailure))) |
There was a problem hiding this comment.
With this, IO#evalOnExecutor has a different behavior from Async#evalOnExecutor. I think they should behave the same.
There was a problem hiding this comment.
They should indeed! I missed that in my review.
There was a problem hiding this comment.
Sorry for this. I hope, I understood you correctly now!
| @@ -35,6 +35,7 @@ import cats.{ | |||
| Traverse | |||
| } | |||
| import cats.data.Ior | |||
| import cats.effect.IO.executionContext | |||
There was a problem hiding this comment.
Instead of importing this can we call it as IO.executionContext? Sometimes unqualified methods can cause confusion 😅
#3633