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

Validate.orElse behavior #1447

Closed
oscar-stripe opened this issue Oct 29, 2016 · 9 comments
Closed

Validate.orElse behavior #1447

oscar-stripe opened this issue Oct 29, 2016 · 9 comments

Comments

@oscar-stripe
Copy link
Contributor

oscar-stripe commented Oct 29, 2016

If we look at validated orElse:
https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/data/Validated.scala#L51

  def orElse[EE, AA >: A](default: => Validated[EE, AA]): Validated[EE, AA] =
    this match {
      case v @ Valid(_) => v
      case Invalid(_) => default
    }

Unlike other uses of Validated, if both are Invalid, we only get the right here. It would be nice to have a combinator like:

  def or[EE: Semigroup, AA >: A](default: => Validated[EE, AA]): Validated[EE, AA] =
    this match {
      case v @ Valid(_) => v
      case Invalid(e1) => default match {
        case v @ Valid(_) => v
        case Invalid(e2) => Invalid(Semigroup[EE].combine(e1, e2))
      }
    }

In fact, I would have thought that is what orElse does. It is a bummer to lose the left error in this case when both have errored.

@non
Copy link
Contributor

non commented Oct 30, 2016

I support the or combinator.

I guess orElse is intended to be used for some kinds of error-recovery, where the left error isn't an error so much as a signal to try something else? I agree it's a bit weird (but does match the behavior of types like Option).

@johnynek
Copy link
Contributor

Well, I would say that the or above is also like Option.orElse: you get an error only if both are errors. The question is what error do you get?

I think it is strange that in the orElse case we use the a + b == b semigroup, but in the product case we use the Semigroup[EE].

Like, if you want the orElse behavior, you could go to Either to get it. I thought the whole point of Validated was to accumulate as many errors as possible in the error case.

@non
Copy link
Contributor

non commented Oct 30, 2016

So I have no idea what people in the world want. I can imagine situations where you do want this kind of monadic behavior (first check A, if it's fine you're done, otherwise check B ignoring A's failure). But you're right that there needs to be a way to do the applicatively that is at least as ergonomic.

I would be fine changing orElse, but I'd like to get wider commentary first, especially since this is a subtle change -- code will continue compiling but do something different. Introducing or is an easy way to support both behaviors (assuming people are relying on the current behavior, which I assume they are). If no one actually wants this behavior then it seems worth changing ASAP.

@travisbrown
Copy link
Contributor

@johnynek Isn't your or just <+>?

scala> import cats.data.{ Validated, ValidatedNel }
import cats.data.{Validated, ValidatedNel}

scala> import cats.syntax.semigroupk._
import cats.syntax.semigroupk._

scala> val bad1: ValidatedNel[String, Int] = Validated.invalidNel("foo")
bad1: cats.data.ValidatedNel[String,Int] = Invalid(NonEmptyList(foo))

scala> val bad2: ValidatedNel[String, Int] = Validated.invalidNel("bar")
bad2: cats.data.ValidatedNel[String,Int] = Invalid(NonEmptyList(bar))

scala> val good1: ValidatedNel[String, Int] = Validated.valid(1)
good1: cats.data.ValidatedNel[String,Int] = Valid(1)

scala> val good2: ValidatedNel[String, Int] = Validated.valid(2)
good2: cats.data.ValidatedNel[String,Int] = Valid(2)

scala> good1 <+> good2
res0: cats.data.Validated[cats.data.NonEmptyList[String],Int] = Valid(1)

scala> bad1 <+> good1
res1: cats.data.Validated[cats.data.NonEmptyList[String],Int] = Valid(1)

scala> bad1 <+> bad2
res2: cats.data.Validated[cats.data.NonEmptyList[String],Int] = Invalid(NonEmptyList(foo, bar))

I'm a little surprised by the current orElse behavior as well—I think I tend to think of it as the standard library's name for <+>, and the fact that they're different here doesn't seem ideal.

@non
Copy link
Contributor

non commented Oct 30, 2016

@travisbrown They are similar, although orElse has a by-name parameter to avoid evaluating the RHS. I think at a minimum we want to add an applicative-or combinator of some type.

@johnynek
Copy link
Contributor

It's always a bummer when we have two functions of the same type signature that are slightly different. I hope no one finds a compelling reason we would not want to use the Semigroup in orElse since it would be nicer to have fewer things to consider.

@non
Copy link
Contributor

non commented Oct 30, 2016

If we were creating this from scratch I agree with you 100%. I want to try to figure out to what degree people are relying on (or need) the current behavior before committing to changing it (or doing something else).

@non
Copy link
Contributor

non commented Oct 30, 2016

Looks like it's been this way since the beginning: https://github.com/typelevel/cats/pull/239/files#diff-729adf389426da6b093a5283e0e2edaeR51

@stew Do you feel strongly that the definition should stay the same?

@mgzuber
Copy link
Contributor

mgzuber commented Nov 3, 2016

I put in PR #1448 before seeing this issue/discussion thread, so I figure I'll add my two cents as someone who is trying to migrate code depending on scalaz to cats.

I can only imagine that the current definition of orElse is based off of scalaz's implementation because they are identical. I agree with @travisbrown and @johnynek that it makes more sense for the orElse definition to be changed, however, we do have a few instances where we have used orElse as currently implemented, and so it would make migration easier if this was kept.

It seems to me that the decision is leaning towards adding an or combinator, so that's what I've done in my PR.

mgzuber added a commit to mgzuber/cats that referenced this issue Nov 3, 2016
mgzuber added a commit to mgzuber/cats that referenced this issue Nov 3, 2016
Also added orThen combinator to preserve old functionality. Closes typelevel#1447
mgzuber added a commit to mgzuber/cats that referenced this issue Nov 4, 2016
Also added orThen combinator to preserve old functionality. Closes typelevel#1447
mgzuber added a commit to mgzuber/cats that referenced this issue Nov 14, 2016
Also added orThen combinator to preserve old functionality. Closes typelevel#1447
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

No branches or pull requests

5 participants