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 ZValidation#toException #603

Open
wants to merge 7 commits into
base: series/1.x
Choose a base branch
from

Conversation

sideeffffect
Copy link
Member

No description provided.

@sideeffffect sideeffffect requested a review from a team as a code owner April 12, 2021 11:09
Copy link
Contributor

@adamgfraser adamgfraser left a comment

Choose a reason for hiding this comment

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

Hmmm, I'm inclined to think we should just implement getOrElse.

There isn't a generic way to convert a collection of E values into a Throwable. We're seeing that just discussing the implementation of these methods. Should it be an IllegalArgumentException or not? Should it include the log? Should it use suppressed exceptions?

Since there isn't a generic way to do it we should just provide operators like fold or getOrElse that allow users to do it as easily as possible.

@sideeffffect
Copy link
Member Author

Sure there is 😃 ZValidationFailureException is 100% the same thing as ZValidation.Failure, only a subtype of Exception, so it has log and whether it is IllegalArgumentException or not is immaterial. The whole point of it is to make it easy to throw. We can think of it as an interop with the wider JVM world.

I have extensive experience with Validated from cats, where I needed a functionality like this fairly often and had to do it myself, which was annoying, so I wanted to improve ZIO Prelude to make it more appealing compared to the competition. If we don't provide users with functions like toException or getOrHandleException, they will have to write more boilerplate code, I don't think I've been the only one with such use case.

The toExceptionWithCause variant is just a cherry on top. It builds on top of the basic variant and gives the user the benefit that the "JVM interop" is improved, but works only if Es are Throwables. I think this extension of the basic variant is worthwhile and will be used in practice, not only because people make their domain errors subtypes of Exception.

Good point about the getOrElse, I've added it. (We already have fold.)

@adamgfraser
Copy link
Contributor

Hmmm. My experience has been that organizations that use Validation also create their own error types when they do need to throw exceptions, though I'm sure practices vary. This seems somewhat ad hoc to me and I don't see it as being so important that we need to do this as opposed to just letting people throw whatever errors they want to based on the information contained in Validation. So I am mildly 👎 on this.

@sideeffffect
Copy link
Member Author

Sometimes it's just easier to throw the exception you're given, without necessarily creating a custom exception yourself for such situation. Sometimes creating a custom exception class is just overkill. With this PR merged, ZIO Prelude would have a choice to do that. Typically, it would be useful in situations where an error is really unexpected and happens only in case of a severe failure.
Example where I would find this helpful is this: https://github.com/Idealiste-cz/ideal-voting-backend/blob/e27c04a/ideal-voting-server/src/main/scala/cz/idealiste/idealvoting/server/Db.scala#L79

But I've create a smaller PR, hopefully less controversial, with parts of functionality of this one:
#610

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.

None yet

2 participants