Add more Parallel instances #1938
Conversation
| @@ -1,10 +1,12 @@ | |||
| package cats | |||
There was a problem hiding this comment.
This file should be renamed to ParallelSuite
| import cats.data._ | ||
| import cats.tests.CatsSuite | ||
| import org.scalatest.FunSuite | ||
| import cats.laws.discipline.{ApplicativeErrorTests, SerializableTests, ParallelTests => ParallelTypeclassTests} | ||
| import cats.laws.discipline.{ApplicativeErrorTests, NonEmptyParallelTests, SerializableTests, ParallelTests => ParallelTypeclassTests} |
There was a problem hiding this comment.
then this import alias won't be needed.
| @@ -1,10 +1,12 @@ | |||
| package cats | |||
|
|
|||
There was a problem hiding this comment.
the package should be cats.tests
|
|
||
| def apply[A](value: Future[A]): FailFastFuture[A] = new FailFastFuture(value) | ||
|
|
||
| def catsDataApplicativeForFailFastFuture(implicit ec: ExecutionContext): Applicative[FailFastFuture] = |
There was a problem hiding this comment.
Just tested, this doesn't seem to pass the ApplicativeLaw.
There was a problem hiding this comment.
Good catch! I'll try to see if it might be a valid Apply instead.
There was a problem hiding this comment.
FastFuture.applicative.ap consistent with product + map *** FAILED ***
FastFuture.applicative.apply composition *** FAILED ***
FastFuture.applicative.followedBy consistent map2 *** FAILED ***
FastFuture.applicative.forEffect consistent map2 *** FAILED ***
FastFuture.applicative.map2/map2Eval consistency *** FAILED ***
FastFuture.applicative.map2/product-map consistency *** FAILED ***
FastFuture.applicative.semigroupal associativity *** FAILED ***
| def apply[A](value: Stream[A]): ZipStream[A] = new ZipStream(value) | ||
|
|
||
| implicit val catsDataAlternativeForZipStream: Alternative[ZipStream] with CommutativeApplicative[ZipStream] = | ||
| new Alternative[ZipStream] with CommutativeApplicative[ZipStream] { |
There was a problem hiding this comment.
This should be lawful right? but we can't test it...that's awkward. how about moving it to alleycats?
There was a problem hiding this comment.
Awkward indeed, I'm not sure if alleycats is the right place, since they can be proven to be lawful in e.g. Haskell, but I agree that it's awkward.
Maybe we should move the CommutativeApplicative instance to Alleycats and keep the CommutativeApply instance here?
There was a problem hiding this comment.
I thought about that but it would cause implicit conflict if you import both right? Unless we do the export hook trick which is kind of deprecated. Maybe we just document a bit more.
| λ[Vector ~> ZipVector](v => new ZipVector(v)) | ||
| } | ||
|
|
||
| implicit def catsStdParallelForZipStream[A]: Parallel[Stream, ZipStream] = |
There was a problem hiding this comment.
same idea as above, if we can't test it, maybe moving to alleycats?
LukaJCB
force-pushed
the
parallel-zip-instances
branch
from
2d40113
to
6c22f90
Compare
LukaJCB
force-pushed
the
parallel-zip-instances
branch
from
1b9dd65
to
0e752e3
Compare
|
Thanks so much! @LukaJCB |
|
this one is ready for merge? @johnynek there are some minor change after your approval. |
|
I am just going to merge this one with 2 sign off |
This PR depends on #1837
It adds a bunch of instances for Zipping collections. Note, that all the non-lazy collections only have an
Applyinstance, as theirApplicativeinstance would be unlawful. It also adds aFailFastFuturewhich does not wait for the other Future to finish should it fail, as suggested by @johnynek here.Here is a list of the added instances:
NonEmptyList[A] <-> ZipNonEmptyList[A]NonEmptyVector[A] <-> ZipNonEmptyVector[A]Vector[A] <-> ZipVector[A]Stream[A] <-> ZipStream[A]List[A] <-> ZipList[A](Parallel[M, F], Alternative[M], Alternative[F]) => OneAnd[M, ?] <-> OneAnd[F, ?](this is really just forNonEmptyStream)Future[A] <-> FailFastFuture[A]