Skip to content
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

Added zipWithPar and derived zipPar, zipParLeft and zipParRight #1029

Merged
merged 3 commits into from
May 4, 2020

Conversation

Adriani277
Copy link
Contributor

closes #1028

@Adriani277
Copy link
Contributor Author

Adriani277 commented May 3, 2020

@neko-kai I know you have mentioned possibly adding a BIOParallel but I was not sure where this would fit in the hierarchy.
I assume BIOParallel would be extended by BIOAsync?

I have 2 commits:

  1. Adding functions to BIOAsync3
  2. An attempt at creating a BIOParallel sub-hierarchy

Please let me know your thoughts on this

def zipWithPar[R, E, A, R1 <: R, E1 >: E, B, C](fa: F[R, E, A], fb: F[R1, E1, B])(f: (A, B) => C): F[R1, E1, C]

// defaults
def parTraverse_[R, E, A, B](l: Iterable[A])(f: A => F[R, E, B]): F[R, E, Unit] = InnerF.void(parTraverse(l)(f))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have noticed that parTraverse_ is not final. Should this be made final? If not, why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Adriani277
This shouldn't be made final because BIOAsyncZio instance overrides it to use ZIO.foreachPar_. This lets it use a native implementation from the effect type that is more optimized and may have enhancements compared to a naive implementation (like, better support for stack-traces, e.g. notice the new ZipLeftFn wrapper in implementation of ZIO.<* - https://github.com/zio/zio/blob/master/core/shared/src/main/scala/zio/ZIO.scala#L108 - this allows the stacktrace where <* occurs to point to the source code line contained inside the user-provided that lambda, instead of the line in ZIO.scala source file that would be there without the new ZipLeftFn)

Generally, the only methods that are final in the hierarchy are those that are trivial enough they can't have better native implementations in the effect type. So, I'd propose to un-final zipParLeft & zipParRight and redirect them to <& and &> in BIOAsyncZio

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for the explanation.

Will make the changes

@neko-kai
Copy link
Member

neko-kai commented May 3, 2020

@Adriani277
I imagined BIOParallel would be entirely off-side, like cats.Parallel, but actually I like what you did much better, it makes a lot more sense, thanks!

Also, the BIOParallel class should be enough to provide a conversion to cats.Parallel in BIOCatsConversions -

trait BIOCatsConversions8 extends BIOCatsConversions9 {
- if you're interested, you could add it in a subsequent PR

@Adriani277
Copy link
Contributor Author

Adriani277 commented May 3, 2020

I can have a look at adding the parallel conversion next.
I would also like to add BIO3Syntax for BIOParallel but I don't understand it enough yet.
Will attempt in the subsequent PR, unless you think BIOParallel should not have syntax added

@neko-kai
Copy link
Member

neko-kai commented May 3, 2020

@Adriani277
Adding syntax to BIOParallel would involve

This is for bifunctor BIOSyntax, you'll also need to repeat the same steps for trifunctor BIOSyntax3.

You'll also need to rename BIOParallel to BIOParallel3 and add a type alias BIOParalell into bio package object here -

type BIOAsync[F[+_, +_]] = BIOAsync3[Lambda[(`-R`, `+E`, `+A`) => F[E, A]]]

@Adriani277
Copy link
Contributor Author

@neko-kai awesome. Will have a go at adding it

@neko-kai neko-kai merged commit b982d5f into 7mind:develop May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add parallel zip to BIOAsync3
2 participants