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

zipOrAccumulate for Raise (waiting for #2912 to be merged) #2880

Closed
wants to merge 5 commits into from

Conversation

serras
Copy link
Member

@serras serras commented Dec 30, 2022

This generalizes #2872 by providing a zipOrAccumulate operation similar to mapOrAccumulate

@serras serras requested a review from a team December 30, 2022 10:10
@github-actions
Copy link
Contributor

github-actions bot commented Dec 30, 2022

Kover Report

File Coverage [46.15%]
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/continuations/Raise.kt 46.15%
Total Project Coverage 43.53%

@serras serras marked this pull request as draft December 30, 2022 14:36
@serras serras changed the title zipOrAccumulate for Raise zipOrAccumulate for Raise (open for discussion) Dec 30, 2022
* Accumulate the errors from running both [action1] and [action2].
*/
@EffectDSL
public inline fun <R, A, B, C> Raise<NonEmptyList<R>>.zipOrAccumulate(
Copy link
Member

Choose a reason for hiding this comment

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

For Either these are just called zip, should we also rename them to zipOrAccumulate? We removed regular zip since it's equivalent to bind for short-circuit behavior and we re-used the zip name for accumulating behavior.

So we should be consistent between Raise and other data types.

Additionally, in Either is has signature action1.zip(action2, block) but it was requested on KotlinLang why it's not Either.zip(action1, action2, block) that would also align both these signatures further. It might even help back-porting them to 1.x.x easier, WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer too that version of zip, so I'm happy to make the change into a different PR, if you want.

@nomisRev
Copy link
Member

I prefer too that version of zip, so I'm happy to make the change into a different PR, if you want.

@serras not sure I understood what you prefer 😅

@serras
Copy link
Member Author

serras commented Jan 31, 2023

I prefer writing Either.zip(action1, action2) { transform }.

@nomisRev what should we do about the zip naming? I feel like changing the one in Either would be too breaking, so maybe it's better to change the one in Raise to zip?

@nomisRev
Copy link
Member

nomisRev commented Jan 31, 2023

I feel like changing the one in Either would be too breaking

@serras It's a new api that is not yet in 1.1.x, so it's non breaking. The current zip we have in 1.1.x is the traditional Apply short-circuiting behavior, whilst this zip and the one added in arrow-2 is accumulating. So we're free to name it however we think is best for this API.

The Either.zip (or Either.zipOrAccumulate) naming and signature can easily be added to 1.2.x without conflicting with existing APIs, so it could aid in the migration process.

@nomisRev
Copy link
Member

@serras serras changed the title zipOrAccumulate for Raise (open for discussion) zipOrAccumulate for Raise (waiting for #2912 to be merged) Feb 3, 2023
@nomisRev
Copy link
Member

nomisRev commented Feb 6, 2023

Just for keeping all history included in the PR history. In the Arrow meeting of 2 February 2023 the decision was made to use Either.zipOrAccumulate for the concrete version of this API. The relevant PR for this work is in #2916.

@nomisRev
Copy link
Member

nomisRev commented Feb 6, 2023

@serras #2916 is merged, so this can be updated and merged 🙌

@serras serras mentioned this pull request Feb 6, 2023
@serras
Copy link
Member Author

serras commented Feb 6, 2023

Closed in favor of #2916

@serras serras closed this Feb 6, 2023
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.

2 participants