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

Add Arrow Core & Fx syntax for Kotlin's Result type #2478

Merged
merged 9 commits into from
Sep 8, 2021
Merged

Conversation

nomisRev
Copy link
Member

@nomisRev nomisRev commented Sep 6, 2021

No description provided.

@nomisRev nomisRev requested a review from a team September 6, 2021 16:34
Copy link
Contributor

@pablisco pablisco left a comment

Choose a reason for hiding this comment

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

Amazing stuff! Thank you for adding this 😍

@pablisco pablisco self-requested a review September 6, 2021 20:23
Copy link
Member

@raulraja raulraja left a comment

Choose a reason for hiding this comment

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

Looks great, @nomisRev. I wonder if we should have Result.bind() inside the either block too when E is Throwable and in the generic block perhaps Result.bind { throwable -> E }.

@nomisRev
Copy link
Member Author

nomisRev commented Sep 6, 2021

Looks great, @nomisRev. I wonder if we should have Result.bind() inside the either block too when E is Throwable and in the generic block perhaps Result.bind { throwable -> E }.

Sounds great, and this is actually very easy to add.

Copy link
Member

@i-walker i-walker left a comment

Choose a reason for hiding this comment

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

👏🏾

h.exceptionOrNull(),
i.exceptionOrNull(),
j.exceptionOrNull(),
)!!.let(::failure)
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a random thought, not blocking of course. I was wondering if it would make it easier to read these zip implementations if we use computations instead of delegate to higher arity functions,

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 choose the most effiecient implementation that was still somewhat readablee and short here. Writing this as a nullable { } will suffer from the same problem. The only way to write this properly is to be fully exhaustive with fold but that exploses quickly.

@nomisRev
Copy link
Member Author

nomisRev commented Sep 7, 2021

I wonder if we should have Result.bind() inside the either block too when E is Throwable

@raulraja, to fully supoort this we need multiple receivers. I added the generic variant.

@nomisRev
Copy link
Member Author

nomisRev commented Sep 7, 2021

Thanks for the great reviews, I updated with all suggested changes if you want to take another look. @pablisco @i-walker @raulraja

@@ -182,7 +182,6 @@ public final class arrow/fx/coroutines/ParTraverse {
public static final fun parSequence (Ljava/lang/Iterable;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
public static final fun parSequence (Ljava/lang/Iterable;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
public static synthetic fun parSequence$default (Ljava/lang/Iterable;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/Continuation;ILjava/lang/Object;)Ljava/lang/Object;
public static final fun parSequenceEither (Ljava/lang/Iterable;Larrow/typeclasses/Semigroup;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
Copy link
Member Author

Choose a reason for hiding this comment

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

These were incorrectly named inside ParTraverseValidated.kt.. Should we deprecate them for 0.13.3?

Copy link
Member

@raulraja raulraja left a comment

Choose a reason for hiding this comment

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

thanks @nomisRev!

@nomisRev nomisRev merged commit d425d47 into main Sep 8, 2021
@nomisRev nomisRev deleted the sv-result branch September 8, 2021 14:31
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.

5 participants