Add or combinator to Validated

[mgzuber]

No description provided.

[kailuowang]

A related discussion is happening in #1447

[codecov-io]

Current coverage is 92.20% (diff: 100%)

Merging #1448 into master will increase coverage by <.01%

@@             master      #1448   diff @@
==========================================
  Files           242        242          
  Lines          3618       3619     +1   
  Methods        3550       3549     -1   
  Messages          0          0          
  Branches         68         70     +2   
==========================================
+ Hits           3336       3337     +1   
  Misses          282        282          
  Partials          0          0          

Powered by Codecov. Last update bffd31f...3102154

[mgzuber]

Ah I see, should I close this PR then, or wait for a decision from that discussion and modify the PR accordingly?

[kailuowang]

Your PR, your call. I just wanted to connect this PR and you to that discussion, in case you might want to take part in.

[johnynek]

I'd rather this be the default behavior, but if others really think that will cause hard bugs I'm okay with or.

One issue: can we comment on each linking to the other? In the comment on orElse contrast it with or and likewise for the opposite?

[mgzuber]

Yup, I added that.

[travisbrown]

I'd also vote for accumulating orElse and a new name for the alternative. We can document it and make the change really prominent in the release notes, but I'd rather not keep around something we all seem to agree is broken just for the sake of consistency with Scalaz or our own (pre-1.0) versions.

[non]

Maybe orThen for the take-right version?

[mgzuber]

I think that's probably the right call, I've changed the code to reflect that.

[johnynek]

👍

[kailuowang]

How about adding some tests?

[non]

Maybe specifically mention that if both instances are failed, only the error on the right is preserved? I think the current sentence is good, but the behavior should also be explicitly explained as well.

[mgzuber]

Added a couple tests and cleaned up the doc on orThen!

[travisbrown]

👍 but I'd vote to hold off on merging this until 0.8.1 is published so that we can keep things simple by publishing off of master. (Even though this would be binary compatible it's a big enough change in behavior that I don't think it should happen in a patch release.)

[johnynek]

+1 to not changing in a patch release.

That said, I do want to point out something here: if the E = Int and you were trying to count the number of failures, what would you want? In that case an or-style combinator would not combine (because you only had 1 failure in this case that blocked the success, you just had two path either of which were acceptable, that blocked).

So, if you wanted to count failures, you would use .orThen not orElse.

Something in the back of my mind makes me think orElse may actually be good as is because it really is or.

What we are talking about here is like or on the value side, but if that can't succeed, and on the error side. I'm starting to wonder if the name orElse is really appropriate for that.

I know, I pushed strongly for this earlier, and the motivation there was consistency: always use the semigroup when you can.

However, the error counting example seems to give me a bit of pause.

Maybe .oneOrCombine? Maybe too literal? Anyway, just wanted to share some second thoughts.

[travisbrown]

@johnynek For me both the consistency and the expectation that <+> and orElse do the same thing (which is true for all other types that have both in Cats) are a strong enough argument for the orElse here.

Even in the case of error counting, I think the accumulating behavior at least arguably makes sense. You actually did try both sides (which may have included attempting to decode something, etc.), and if I'm using Validated I think I'd tend to expect that all attempts are taken into account on the failure side. If I want just the failures on the path to a result, I'd use Either.

(Also I was wrong about this being binary compatible in my previous comment, since there's the new Semigroup requirement.)

[johnynek]

@travisbrown good point. Consistency is a strong argument. +1.

[travisbrown]

Hmm, I just ran into a fact that's making me question my original position (in favor of accumulating orElse).

In circe decoders can be run in either fail-fast or error-accumulating mode, and there's a requirement that simply dropping all errors except the first in error-accumulating mode should always give the same result as fail-fast mode.

This holds for all decoders defined in the library except for ones built with or (which I recently changed to have the same behavior as the new orElse here).

I'm not sure this is much of an argument one way or the other, but it does feel a little suspicious.

[johnynek]

I'm actually not sure orElse and <+> consistency is the right thing. For instance, if I defined:

class List[T] {
  def orElse[U >: T](that: List[U]): List[U] =
    if (nonEmpty) this
    else that
}

we would not want consistency with <+> and orElse. So, maybe we would say this is a bad method name, but presuming orElse == <+> is putting a strong semantic bias on what <+> means since orElse calls out to be an analog of a | b while <+> does not really.

[mgzuber]

Is there a stronger consensus here? I can change what I've got if the opinion is that the orElse signature shouldn't change.

[travisbrown]

@mgzuber At this point I don't feel strongly either way—I'm no longer convinced that either implementation is more consistent or elegant, so as long as they're both available I don't think the names matter all that much.

[johnynek]

a very clear approach would be to add def orBothInvalid[B >: A](that: Validated[E, B])(implicit sg: Semigroup[E]): Validated[E, B]

or something like that. The name should make it clear that at least this one gives both invalids.

[mgzuber]

@johnynek that would be in tune with the scalaz approach (the method name is findSuccess). I don't totally agree with it and I'm inclined to say that orElse seems more correct because it resembles what I would expect going from Option to Validated.

That being said, something like findValid might make it easier for people to migrate over from scalaz to cats.

[johnynek]

So, in the absence of a strong argument one way or the other, I think the fallback of API stability may win the day in my book.

So, I would say +1 to findValid to at least evoke scalaz.

[mgzuber]

Done

[kailuowang]

👍 Thanks very much!

[johnynek]

👍