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

Propose Try.toEither(Throwable => L) #2724

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eslep
Copy link

@eslep eslep commented Oct 28, 2022

Thank You

Thank you for all of the work on Vavr! It has quickly become my favorite Java library, and usually the first thing I add to every new project ❤️ Aside from the benefit to my own code, it has also been a great tool to help teach others about FP and get them curious about it.

Proposal / What is this?

I come across cases where I want to convert a Try to an Either while mapping the Failure value onto a Left value.
One example would be safely handling code which isn't mine, then mapping the Throwable onto an error type.

I do this normally either with fold(throwableMapper, identity()) or .toEither().mapLeft(throwableMapper), but find that .toEither(throwableMapper) would be a nice shortcut to replace either case.
I have also a few colleagues who would appreciate the addition from a readability standpoint I think, so I thought I'd suggest it.

Implementation

By my own style, I'd normally just write this as a wrapper around a fold call. However, as a first contribution, I am trying to stick as close as possible to the style I already see in the code here.

By that reasoning, the code style, namings, and Javadoc are all created based on examples I find existing within the file.

Since the toEither(Supplier) implementation assumes the Supplier is pure/will not throw an exception and does not handle such a case, I have followed the same assumption here as well.

Copy link
Member

@nfekete nfekete left a comment

Choose a reason for hiding this comment

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

Looks good to me, with a few minor suggestions.

Comment on lines 975 to 978
final Either<Boolean, Object> either = failure.toEither(t ->
t.getMessage().equals("a certain value")
);
assertThat(either).isEqualTo(Either.left(true));
Copy link
Member

@nfekete nfekete Oct 31, 2022

Choose a reason for hiding this comment

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

I'd probably replace the test code in a way that limits the number of comparisons to one, which would be the one in the assertion itself.
You could use a mapper function that extracts the message from the exception and use that as the left value to compare to in the assertion. This would make the code both shorter, and — in my opinion — easier to understand, since it would have a slightly less degree of indirection.

Suggested change
final Either<Boolean, Object> either = failure.toEither(t ->
t.getMessage().equals("a certain value")
);
assertThat(either).isEqualTo(Either.left(true));
final Either<Boolean, Object> either = failure.toEither(Throwable::getMessage);
assertThat(either).isEqualTo(Either.left("a certain value"));

Copy link
Author

Choose a reason for hiding this comment

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

Sure, sounds much better that way 👍 I also felt a bit weird using Throwable => Boolean as my function where many people would maybe expect that to then be Predicate<Throwable>. I can see also how the equals looks like an assertion.

Main point was just to have a clear expected value returned by the function, so the suggestion sounds great

@@ -1132,6 +1132,21 @@ public final Either<Throwable, T> toEither() {
}
}

/**
* Converts this {@code Try} to an {@link Either}, converting the Throwable (if present)
Copy link
Member

Choose a reason for hiding this comment

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

I'm proposing this change for the documentation so that it is more precise in meaning, because one could legitimately use a Try value to wrap a Throwable instance as a success value.

Suggested change
* Converts this {@code Try} to an {@link Either}, converting the Throwable (if present)
* Converts this {@code Try} to an {@link Either}, converting the Throwable (in the failure case)

Copy link
Author

Choose a reason for hiding this comment

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

I had the same impression that the wording was strange, but I kept this to stay consistent with toValidation(Throwable => U).

I would make this doc change to both of these methods then, since your point applies to both cases.

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