-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Introduce parReplicateA #4007
Introduce parReplicateA #4007
Conversation
/** | ||
* Like [[Applicative.replicateA]], but uses the apply instance | ||
* corresponding to the Parallel instance instead. | ||
*/ | ||
def parReplicateA[A](n: Int, ma: M[A]): M[List[A]] = | ||
if (n <= 0) monad.pure(List.empty[A]) | ||
else Parallel.parSequence(List.fill(n)(ma))(UnorderedFoldable.catsTraverseForList, this) |
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.
To me it seems that trait Parallel
is designed to just represent a relationship between a Monad and Applicative with parallel composition. But most of complimentary methods are placed into object Parallel
(including parAp
per se). I'm not absolutely confident, but shouldn't parReplicateA
be also placed in the object rather than the trait?
if (n <= 0) P.monad.pure(List.empty[A]) | ||
else Parallel.parSequence(List.fill(n)(ma)) |
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 guess these two lines can be replaced with just P.parReplicateA(...)
if you decide to leave Parallel#parReplicateA
in the trait Parallel
or with Parallel.parReplicateA(...)
if you move it into the object. Just to avoid code duplication.
@@ -184,6 +184,7 @@ object SyntaxSuite { | |||
val mb4: M[B] = ma.parProductR(mb) | |||
val mab2: M[(A, B)] = ma.parProduct(mb) | |||
val mb5: M[B] = mab.parAp(ma) | |||
val mla: M[List[A]] = ma.parReplicateA(3) |
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.
mock[Int]
instead of just 3
?
Hello @satorg I agree with you comments and so I've pushed corresponding changes. |
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.
Looks good to me, thanks.
Although I think it will be also appreciated if you add a corresponding test case to ParallelSuite
.
Hello @satorg I added some tests as you've requested. |
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 looks great, thanks! :)
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.
Looks great, thx!
* Like `Applicative[F].replicateA`, but uses the apply instance | ||
* corresponding to the Parallel instance instead. | ||
*/ | ||
def parReplicateA[M[_], A](n: Int, ma: M[A])(implicit P: Parallel[M]): M[List[A]] = |
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.
why not P.applicative.replicateA(n, ma)
?
Since in principle, that implementation could contain some optimization/specialization of some kind.
I think that is a better than having a second implementation here.
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.
It has to be a bit more complicated than that.
P.sequential(P.applicative.replicateA(n, P.parallel(ma)))
But yes, I agree, it's better to reuse it.
This PR introduces
parReplicateA
as a parallel version ofApplicative.replicateA
.