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#getOrElse and ZValidation.Failure#message #610

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

Conversation

sideeffffect
Copy link
Member

Less controversial take on #603

/**
* Returns the value, if successful, or the transformed (using `f`) failure.
*/
final def getOrElse[A1 >: A](f: Failure[W, E] => A1): A1 = this match {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe one variant that just gives you the errors and another getOrElseLog variant that gives you a tuple of the log and the errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

With Failure, you may get your hands on message, which is my point here 😸

But I can add another method that will take just f: NonEmptyChunk[E] => A1. Do you think, it would be useful?

Copy link
Contributor

Choose a reason for hiding this comment

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

It just seems a little irregular to expose one of the constructors in a fold variant as opposed to the underlying data values, at least where it isn't a fixed point type. Maybe a prettyPrint or render method on ZValidation could address this use case?

Copy link
Member Author

Choose a reason for hiding this comment

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

What I'm trying to achieve here, is to make it super easy to do something like this:

.getOrElse(e => sys.error(e.message)) // this whole thing returns the successful value (or throws)

So I need to have the member message on Failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we put it on ZValidation itself so it is like validation.getOrElse(_ => sys.error(validation.render))? Definitely want to make it easy to do something like that. Just having separate operators on the subtype of an algebraic data type and returning the subtype instead of its contents in a fold seems like a bit of a code smell.

Copy link
Member Author

Choose a reason for hiding this comment

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

But then you have to use the variable (validation in your example) twice. In a situation, when you're juggling with multiple values of Validation, you have to make special care that you use the right one at the right place -- that's an unnecessary room for errors.

What I'm trying to replicate is an analogue of valueOr from cats:
https://typelevel.org/cats/api/cats/syntax/EitherOps.html#valueOr[BB%3E:B](f:A=%3EBB):BB

I've renamed it, so that it better reflects the meaning, getOrElse is already established for a slightly different concept.

lazy val errorsUnordered: NonEmptyMultiSet[E] = NonEmptyMultiSet.fromIterable(errors.head, errors.tail)

/** The description of the failure. */
lazy val message: String =
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this covered by Debug instance?

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 think the current Debug instance is all-right as it is, but message is formatted with newlines and so on to enhance readability, which I think is very useful for many purposes.

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.

Looks good! Just a couple of minor comments.

@sideeffffect
Copy link
Member Author

@adamgfraser I know you're super busy and this is not urgent, but once you have the time, could you please have another look at this, please 🙏

@adamgfraser
Copy link
Contributor

@sideeffffect Yes! Sorry about that! Just commented.

/**
* Returns the value, if successful, or the transformed (using `f`) failure.
*/
final def valueOr[A1 >: A](f: Failure[W, E] => A1): A1 = this match {
Copy link
Member Author

Choose a reason for hiding this comment

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

Or we could call this handleFailure and it would get right the formatted string, as in

  final def handleFailure[A1 >: A](f: String => A1): A1 = ???

But that felt a bit sketchy...

So this valueOr is the best thing I was able to come up with that also satisfies the criteria of ease of use and idiot-proofness.

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