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

Update Either.zipOrAccumulate #2959

Closed
wants to merge 22 commits into from
Closed

Conversation

nomisRev
Copy link
Member

@nomisRev nomisRev commented Mar 3, 2023

New Description

Note: Everything mentioned here is open for discussion, so please provide all feedback.

We've introduce earlier Either.zipOrAccumulate for combine + Either, Either & EitherNel. This however doesn't allow intermixing of Either & EitherNel in the same function and thus requires manual lifting in some cases. Defining all different combination would be almost impossible ((9^2)*2 = 162 methods). i.e.

Either.zipOrAccumulate(String::plus, "error".leftNel(), 1.right()) { x: Int, y: Int -> x + y }
Either.zipOrAccumulate(String::plus, "error".leftNel(), 1.right()) { x: Int, y: Int -> x + y }
Either.zipOrAccumulate(1.right(), "error".leftNel()) { x: Int, y: Int -> x + y }
Either.zipOrAccumulate("error".leftNel(), 1.right()) { x: Int, y: Int -> x + y }

Without adding the 162 methods users need to manually do, although this is the same as before with Validated.

Either.zipOrAccumulate(String::plus, "error".leftNel(), 1.right().toEitherNel()) { x: Int, y: Int -> x + y }
Either.zipOrAccumulate(String::plus, "error".leftNel(), 1.right().toEitherNel()) { x: Int, y: Int -> x + y }
Either.zipOrAccumulate(1.right().toEitherNel(), "error".leftNel()) { x: Int, y: Int -> x + y }
Either.zipOrAccumulate("error".leftNel(), 1.right().toEitherNel()) { x: Int, y: Int -> x + y }

To solve this problem Arrow introduced a DSL zipOrAccumulate API that allows binding both Raise<E> and Raise<NonEmptyList<E>> and thus also Either<E, ?> and EitherNel<E, ?> This serves as a replacement for the accumulate error pattern of Validated<E, ?> and ValidatedNel<E, ?>.

However this doesn't allow to pass Either values, and thus the API for Either is the same as Raise with the exception of it getting nesting in the DSL.

either {
  zipOrAccumulate({ "error".leftNel().bindNel() }, { 1.right().bind() }) { x: Int, y: Int -> x + y }
} shouldBe Either.zipOrAccumulate({ "error".leftNel().bindNel() }, { 1.right().bind() }) { x: Int, y: Int -> x + y }

So this brings the question:

  • Do we want to introduce this DSL based API for Either.Companion
  • Do we want to force users to manually wrap in either { }
  • Or do we want to stick to the current status-quo requiring toEitherNel() and work value based? (Note that the current value based implementation could still be derived from the DSL one).

Side note: zip deprecation

Arrow is deprecating Either.zip(fa, fb, f) in favour of either { f(fa.bind(), fb.bind()) } since it doesn't suffer from the arity-n problem, and offers the exact same behavior. Since either is now inline fun in 1.2.0 and thus works with-or-without suspend it can be used as a drop-in replacement. This has happened so far for Either, Option and still needs to happen for Ior, Result & Nullable.😅 Unless we're coming back from that (?), the deprecation has not been released yet.

Old description

This PR updates Either.zipOrAccumulate but sadly since we don't have context receivers yet, and have to emulate through RaiseAccumulate it breaks inference / BuilderInference in a worse way than the PR that started this effort, #2952. This PR the signature with parZipOrAccumulate.

We can of-course put off this change, and keep the current signatures until we have context-receivers. The migration path for it is relatively simple, and it will be easily to automate. With ReplaceWith (or OpenRewrite).

More details below:

If we put @BuilderInference on the method, instead of the individual functions we get following error:

Screenshot 2023-03-03 at 14 43 56

Some related YouTrack tickets related to the IDEA message:

If we put @BuilderInference we get following error for every lambda that tries to bind EitherNel<E> instead of Either<E> and will require to either specify the type arguments in the function, or specify a return type so E can be inferred from there.
Screenshot 2023-03-03 at 16 21 37

The up sight, with context receivers it looks 🔥 😍

Screenshot 2023-03-03 at 16 17 47

@nomisRev nomisRev added the 1.2.0 Tickets belonging to 1.1.2 label Mar 3, 2023
@nomisRev nomisRev requested a review from serras March 3, 2023 15:06
@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2023

Kover Report

File Coverage [53.02%]
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/Either.kt 53.02%
Total Project Coverage 44.08%

@nomisRev nomisRev force-pushed the sv-update-raiseaccumulate branch from a88a9d0 to 1129b23 Compare March 6, 2023 12:56
@nomisRev nomisRev force-pushed the sv-update-raiseaccumulate branch from 1129b23 to 9b3889c Compare March 6, 2023 13:09
@nomisRev nomisRev force-pushed the sv-update-raiseaccumulate branch from 70e831c to 0239aaa Compare March 6, 2023 13:51
@nomisRev nomisRev force-pushed the sv-update-either-zipOrAccumulate branch from ec4abeb to 03f2910 Compare March 6, 2023 14:26
@nomisRev
Copy link
Member Author

nomisRev commented Mar 6, 2023

The issue described above has been fixed for this API by a suggestions made by @Zordid.

Screenshot 2023-03-06 at 15 27 33

@nomisRev nomisRev force-pushed the sv-update-raiseaccumulate branch from 0239aaa to 5ea2cf4 Compare March 6, 2023 16:05
@nomisRev nomisRev force-pushed the sv-update-either-zipOrAccumulate branch from edb2407 to 9994be8 Compare March 6, 2023 16:06
@nomisRev nomisRev requested review from franciscodr and raulraja March 7, 2023 08:38
Base automatically changed from sv-update-raiseaccumulate to main March 8, 2023 07:38
@raulraja raulraja closed this Mar 8, 2023
@nomisRev nomisRev reopened this Mar 8, 2023
@nomisRev
Copy link
Member Author

Closing this PR since we want to keep the Either value based API as it stands in Validated.

@nomisRev nomisRev closed this Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.2.0 Tickets belonging to 1.1.2 discussion help wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants