-
Notifications
You must be signed in to change notification settings - Fork 28
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
Don’t ignore mappend failures in validation #39
base: master
Are you sure you want to change the base?
Don’t ignore mappend failures in validation #39
Conversation
Previously, a <> b would give success if either a or b were a success! This should not happen when doing validation. We want any errors to be propagated. Fixes system-f#35
058c795
to
be86a2b
Compare
There is no good way to define a Monoid instance for Validation that retains failures. The two identity rules required for Monoid instances are: - mempty <> x = x - x <> mempty = x There is only one possible definitions of mempty that meets these requirements, and preserves errors: - mempty = Success mempty However, this would require that the Right side of Validation have a Monoid instance. Someone could add this instance with a Monoid a constaint, however, it is not so useful because the Right side is usually not a Monoid.
Don’t rely on lens #
appValidation _ (Failure e1) (Success _) = | ||
Failure e1 | ||
appValidation _ (Success _) (Failure e2) = | ||
Failure e2 | ||
appValidation _ (Success a1) (Success _) = | ||
Success a1 |
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.
This case is still awkward. Why drop the right side here?
Why not? instance (Semigroup e, Semigroup a) => Semigroup (Validation e a) where
x <> y = liftA2 (<>) x y
instance (Semigroup e, Monoid a) => Monoid (Validation e a) where
mempty = Success mempty The currently left biased implementation using |
It looks like the process of combining two values of the
I wonder, would it be possible to reproduce all behaviours by using proper |
This behavior seems very confusing to me. I would expect someone to use
Yes please. This is satisfied by a
This seems like a
Yes please. This is also satisfied by a |
This requires that the success side is a semigroup. That seems pretty rare case to me (and bound to be confusing). Looking at the examples, the success side is a semigroup only very rarely. In the cases where it is, I'm not sure if you want this. For instance: (Success "abc") <> (Success "def") = Success "abcdef" This works but seems surprising. I would much rather it just take one of the successes. But, I would agree it's probably better than the status quo. |
This is actually how Either works. For instance: Left "error" <> Right "success" = Right "success" I suspect this is why validation implements semigroup that way. I would argue that it's wrong for Either as well and instead should be Left "error" *> Right "success" = Left "error" But either doesn't use liftA2 here. I wonder if there is any discussion on why that is? |
Ugh, I forgot that was the instance for |
I found this thread on Either's semigroup instance: https://mail.haskell.org/pipermail/libraries/2018-May/028826.html /cc @andrewthad |
The
Why? It generalizes the other option:
We can recover this second instance with
The example you bring up involving
Anyway, I don't particularly care what this library does since it's easy to just roll my own type whenever I need this, but that's my preference. |
Just in case you happen to use |
Every type actually has a trivial semigroup which just throws away one argument. We can access this using the |
Yeah perhaps there are enough newtype's here that we don't need a semigroup instance at all. I think pretty much any change is better than what it's currently doing, so feel free to open a PR doing something else. This is just the change that seems most logical and least likely to break things to me. |
Previously, a <> b would give success if either a or b were a success!
This should not happen when doing validation. We want any errors to be
propagated.
Fixes #35
/cc @tonymorris @gwils