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.

Sign up for GitHub

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

Jump to bottom

Order for Ior #3554

Merged
merged 5 commits into from
Aug 10, 2020
Merged

Order for Ior #3554

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
12cd435
Order for Ior
TimWSpence Aug 7, 2020
c300a91
Move compare definition onto Ior itself
TimWSpence Aug 7, 2020
926a6db
Fix typo
TimWSpence Aug 7, 2020
2182462
Reduce allocations in compare
TimWSpence Aug 7, 2020
64de92f
Make Both > Right
TimWSpence Aug 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions core/src/main/scala/cats/data/Ior.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,17 @@ sealed abstract class Ior[+A, +B] extends Product with Serializable {
(a, b) => that.fold(a2 => false, b2 => false, (a2, b2) => AA.eqv(a, a2) && BB.eqv(b, b2))
)

final def compare[AA >: A, BB >: B](that: AA Ior BB)(implicit AA: Order[AA], BB: Order[BB]): Int =
Copy link
Contributor

@travisbrown travisbrown Aug 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think it might be useful to have a comment with a high-level explanation of the semantics here. I think what you have makes sense as the natural way to do this, but what exactly it's doing isn't necessarily clear at a glance.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest, I copied the semantics of === above and didn't think too much about it! I guess it's just standard practice for a covariant datatype?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's mostly the between-ness of Both that I think isn't necessarily obvious. I could imagine both Left and Right preceding Both, for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Sorry, by "semantics here" I meant the details of how the comparison happens, not the AA >: A part.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sorry!! Yeah I had a look in Haskell and found https://hackage.haskell.org/package/these-1.1.1.1/docs/Data-These.html. It puts Both after Right so I'm happy to change this. Although I don't know how widely used (and therefore canonical) it is. In my head, I feel like Both is kind-of between Left and Right so the ordering makes sense to me but I'm happy to be persuaded otherwise!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, my vote would be to follow the Haskell instance, then, but I don't have a strong opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is an argument for the These choice in that if you think of this as an (Option, Option) pair with two Nones prohibited, both Left and Right would precede Both.

Copy link
Member

Choose a reason for hiding this comment

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

The Haskell instance is derived and I think not a product of deep thought, but I also don't see a compelling reason to diverge from prior art. I think of the type as a cleaner Either[A, (Option[A], B)], which Both after Right is consistent with.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, I'll change it. Thanks for the feedback! :)

(this, that) match {
case (Ior.Left(a1), Ior.Left(a2)) => AA.compare(a1, a2)
case (Ior.Left(_), _) => -1
case (Ior.Both(a1, b1), Ior.Both(a2, b2)) => Order[(AA, BB)].compare((a1, b1), (a2, b2))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kind of nitpicky but it would be pretty easy to avoid the allocation of a new Order instance here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it's worth it for the loss of clarity? (Genuine question, not passive-aggressive 😂 )

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at it again, it's actually at least three allocations (for the instance and then the tuples). Given that one of the places this could be used is for sorting collections, I'd be inclined to say three fewer object allocations per comparison is worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I wouldn't block the PR for this, though.)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

case (Ior.Both(_, _), Ior.Left(_)) => 1
case (Ior.Both(_, _), Ior.Right(_)) => -1
case (Ior.Right(b1), Ior.Right(b2)) => BB.compare(b1, b2)
case (Ior.Right(_), _) => 1
}

final def show[AA >: A, BB >: B](implicit AA: Show[AA], BB: Show[BB]): String =
fold(
a => s"Ior.Left(${AA.show(a)})",
Expand Down Expand Up @@ -752,9 +763,10 @@ sealed abstract private[data] class IorInstances extends IorInstances0 {
}
}

implicit def catsDataEqForIor[A: Eq, B: Eq]: Eq[A Ior B] =
new Eq[A Ior B] {
def eqv(x: A Ior B, y: A Ior B): Boolean = x === y
implicit def catsDataOrderForIor[A: Order, B: Order]: Order[A Ior B] =
new Order[A Ior B] {

def compare(x: Ior[A, B], y: Ior[A, B]): Int = x.compare(y)
}

implicit def catsDataShowForIor[A: Show, B: Show]: Show[A Ior B] =
Expand Down Expand Up @@ -879,6 +891,12 @@ sealed abstract private[data] class IorInstances0 {
override def map[B, C](fa: A Ior B)(f: B => C): A Ior C =
fa.map(f)
}

implicit def catsDataEqForIor[A: Eq, B: Eq]: Eq[A Ior B] =
new Eq[A Ior B] {

def eqv(x: A Ior B, y: A Ior B): Boolean = x === y
}
}

sealed private[data] trait IorFunctions {
Expand Down
4 changes: 3 additions & 1 deletion tests/src/test/scala/cats/tests/IorSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package cats.tests
import cats.{Bitraverse, MonadError, Semigroupal, Show, Traverse}
import cats.data.{EitherT, Ior, NonEmptyChain, NonEmptyList, NonEmptySet}
import cats.kernel.{Eq, Semigroup}
import cats.kernel.laws.discipline.SemigroupTests
import cats.kernel.laws.discipline.{OrderTests, SemigroupTests}
import cats.laws.discipline.{
BifunctorTests,
BitraverseTests,
Expand Down Expand Up @@ -37,6 +37,8 @@ class IorSuite extends CatsSuite {
checkAll("BitraverseTests Ior[*, *]", BitraverseTests[Ior].bitraverse[Option, Int, Int, Int, String, String, String])
checkAll("Bitraverse[Ior]", SerializableTests.serializable(Bitraverse[Ior]))

checkAll("Order[Ior[A: Order, B: Order]]", OrderTests[Ior[Int, Int]].order)

checkAll("Semigroup[Ior[A: Semigroup, B: Semigroup]]", SemigroupTests[Ior[List[Int], List[Int]]].semigroup)
checkAll("SerializableTest Semigroup[Ior[A: Semigroup, B: Semigroup]]",
SerializableTests.serializable(Semigroup[Ior[List[Int], List[Int]]])
Expand Down