-
Notifications
You must be signed in to change notification settings - Fork 115
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
Conversation
/** | ||
* Returns the value, if successful, or the transformed (using `f`) failure. | ||
*/ | ||
final def getOrElse[A1 >: A](f: Failure[W, E] => A1): A1 = this match { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
@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 🙏 |
@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 { |
There was a problem hiding this comment.
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.
Less controversial take on #603