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

["Request"] Consider overloading Arrow Fx ParZip for tupling #2821

Closed
nomisRev opened this issue Sep 9, 2022 · 11 comments
Closed

["Request"] Consider overloading Arrow Fx ParZip for tupling #2821

nomisRev opened this issue Sep 9, 2022 · 11 comments
Assignees
Labels
2.0.0 Tickets / PRs belonging to Arrow 2.0 beginner friendly

Comments

@nomisRev
Copy link
Member

nomisRev commented Sep 9, 2022

When working with parZip you often want to combine it with Effect, rather than exposing many variants of the API these API compose naturally. In Arrow 1.x.x we decided not to overload parZip with a default lambda for the size of the binary.

Since Arrow 2.0 will significantly reduce the size of the binary, we should consider adding these overloads again. For example, when ignoring results or turning values into a tuple it currently requires some annoying overhead.

either<E, Unit> {
  parZip(
    { computation().bind() },
    { computation().bind() }
  ) { _,_ -> }
}

either<E, Pair<Unit, Unit>> {
  parZip(
    { computation().bind() },
    { computation().bind() }
  ) { a, b -> Pair(a, b) } // ::Pair lambda reference is not allowed here due to the `CoroutineScope` receiver.
}

See discussion on Slack: https://kotlinlang.slack.com/archives/C5UPMM0A0/p1662715323510559

@nomisRev nomisRev added the 2.0.0 Tickets / PRs belonging to Arrow 2.0 label Sep 9, 2022
@lgtout
Copy link
Collaborator

lgtout commented Dec 25, 2022

@nomisRev Something like this, as an addition, for each parZip?
Plus the same for each parZip that takes a CoroutineContext?

public suspend inline fun <A, B> parZip(
  crossinline fa: suspend CoroutineScope.() -> A,
  crossinline fb: suspend CoroutineScope.() -> B,
):  Pair<A, B> = parZip(Dispatchers.Default, fa, fb) { a, b ->
    Pair(a, b)
}

Also...
Is it okay to use main as the source branch?
What will be the destination branch for the PR?

@nomisRev
Copy link
Member Author

nomisRev commented Jan 5, 2023

Hey @lgtout,
Sorry for the late reply!

Plus the same for each parZip that takes a CoroutineContext?

Actually, we're going to remove these signatures and they should just take context: CoroutineContext = EmptyCoroutineContext as an argument. As done in this PR for parTraverse. This is in line with what KotlinX Coroutines does and allows for better optimisations by Kotlin.

So the functions should be exactly as you've shown here:

public suspend inline fun <A, B> parZip(
  context: CoroutineContext = EmptyCoroutineContext,
  crossinline fa: suspend CoroutineScope.() -> A,
  crossinline fb: suspend CoroutineScope.() -> B,
):  Pair<A, B> = parZip(context, fa, fb) { a, b ->
    Pair(a, b)
}

This should be done for Pair, Triple, Tuple4-9.

Thanks for looking into this ticket @lgtout. You can just target this to main btw, since it's non-breaking ☺️

@lgtout
Copy link
Collaborator

lgtout commented Jan 5, 2023

@nomisRev Got it. Thanks!

@lgtout
Copy link
Collaborator

lgtout commented Jan 8, 2023

@nomisRev Existing parZips has overloads with up to 8 function parameters (types A-H). But you said you also want to return Tuple 9. So should I add the missing overloads with 9 function parameters? Thanks!

@nomisRev
Copy link
Member Author

nomisRev commented Jan 9, 2023

Yes, that would be great @lgtout! Thank you 🙏

lgtout added a commit to lgtout/arrow that referenced this issue Jan 14, 2023
Support Pair, Triple, and Tuple4-9 return types.
@lgtout
Copy link
Collaborator

lgtout commented Jan 14, 2023

@nomisRev Should I write tests for the added parZips? I see that there are tests for the existing ones, but they seem to be generated by Knit from the documentation. And they don't appear to actually verify anything. They just print the result of running each parZip. Thanks!

@nomisRev
Copy link
Member Author

@lgtout
Copy link
Collaborator

lgtout commented Jan 16, 2023

@nomisRev Okay, that works.
But why are these tests named "parMap" when they all test "parZip"?
And we'll still be missing tests for the new parZip that I added that accepts 9 functions. Should I add those, based on the structure of the existing ones?

@nomisRev
Copy link
Member Author

Hey @lgtout,
The name parMap is legacy, feel free to flatten the tests directly into arrow.fx.coroutines or rename the folder to parZip 👍
Yes, adding the tests for the 9-arity function would also be great! 🙏

Thank you for being so thorough ☺️ 👏 🙌

@lgtout
Copy link
Collaborator

lgtout commented Jan 16, 2023

@nomisRev

feel free to flatten the tests directly into arrow.fx.coroutines or rename the folder to parZip 👍
Yes, adding the tests for the 9-arity function would also be great! 🙏

Will do!

Thank you for being so thorough ☺️ 👏 🙌

😄 Always!

lgtout added a commit to lgtout/arrow that referenced this issue Jan 17, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 17, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 17, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 20, 2023
Support Pair, Triple, and Tuple4-9 return types.
lgtout added a commit to lgtout/arrow that referenced this issue Jan 20, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 20, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 20, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 20, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 20, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 20, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 20, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 20, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 20, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 20, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 22, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 29, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 29, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 29, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 29, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 29, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 29, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Jan 29, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
Support Pair, Triple, and Tuple4-9 return types.
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 5, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 6, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 6, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 6, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 6, 2023
lgtout added a commit to lgtout/arrow that referenced this issue Feb 6, 2023
nomisRev added a commit that referenced this issue Feb 6, 2023
Co-authored-by: Julian Abiodun <lgtout@users.noreply.github.com>
Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
@nomisRev
Copy link
Member Author

nomisRev commented Feb 8, 2023

This is fixed by @lgtout in #2900

@nomisRev nomisRev closed this as completed Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0.0 Tickets / PRs belonging to Arrow 2.0 beginner friendly
Projects
None yet
Development

No branches or pull requests

3 participants