Order for Ior #3554
Order for Ior #3554
Conversation
|
Apologies if there's a reason why this doesn't already exist but I couldn't think of one! |
|
Also I'll follow up with an instance for IorT if this is wanted :) |
Codecov Report
@@ Coverage Diff @@
## master #3554 +/- ##
==========================================
- Coverage 91.30% 91.28% -0.02%
==========================================
Files 386 386
Lines 8565 8575 +10
Branches 248 261 +13
==========================================
+ Hits 7820 7828 +8
- Misses 745 747 +2 |
|
There was a problem hiding this comment.
A couple of minor comments, but this looks reasonable to me. Out of curiosity, is there prior art for this instance, from Haskell or wherever? Having Both be "between" the other two makes sense, I guess, but I'd feel a little better about it if I knew it was the usual decision here.
| (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)) |
There was a problem hiding this comment.
This is kind of nitpicky but it would be pretty easy to avoid the allocation of a new Order instance here.
There was a problem hiding this comment.
Do you think it's worth it for the loss of clarity? (Genuine question, not passive-aggressive 😂 )
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(I wouldn't block the PR for this, though.)
| @@ -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 = | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(Sorry, by "semantics here" I meant the details of how the comparison happens, not the AA >: A part.)
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Hmm, my vote would be to follow the Haskell instance, then, but I don't have a strong opinion.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Cool, I'll change it. Thanks for the feedback! :)
No description provided.