Clean Up Predicate A Bit #269
Conversation
Alias existing functions from symbolic ones Add contravariant instance Guarantee all instances serializable
| implicit def predicateMonoid[A]: Monoid[Predicate[A]] = new Monoid[Predicate[A]] { | ||
| override def empty: Predicate[A] = Predicate.empty | ||
| override def combine(l: Predicate[A], r: Predicate[A]): Predicate[A] = l union r | ||
| } | ||
|
|
||
| implicit val predicateInstance: MonoidK[Predicate] = new MonoidK[Predicate] { | ||
| implicit val predicateMonoidK: MonoidK[Predicate] = new MonoidK[Predicate] { |
There was a problem hiding this comment.
Dunno if this rename is cool from a bincompat perspective.
There was a problem hiding this comment.
There are no guarantees about that yet, so it's fine.
There was a problem hiding this comment.
I think we should add mima checking sooner than later.
I personally really avoid libraries that don't try to minimize breaks (and in fact, despite contributing to this library, I am afraid to depend on it).
There was a problem hiding this comment.
Happy to call it 1.0 at some point in the near future.
Codecov Report
@@ Coverage Diff @@
## master #269 +/- ##
=========================================
+ Coverage 91.19% 91.5% +0.31%
=========================================
Files 25 24 -1
Lines 1612 1625 +13
Branches 337 214 -123
=========================================
+ Hits 1470 1487 +17
+ Misses 142 138 -4
Continue to review full report at Codecov.
|
| override def contramap[A, B](fb: Predicate[A])(f: B => A): Predicate[B] = | ||
| Predicate(f andThen fb.apply) | ||
| override def product[A, B](fa: Predicate[A], fb: Predicate[B]): Predicate[(A, B)] = | ||
| Predicate(v => fa(v._1) || fb(v._2)) |
There was a problem hiding this comment.
I'm curious about the choice of || here, what's the reasoning?
There was a problem hiding this comment.
Yeah, it's definitely a bit arbitrary, but I made the choice based on having the monoidal product act similarly to the monoids already defined for this type. || gives us union, which is probably the more in line with user expectations given the monoids are also union.
There was a problem hiding this comment.
Could you please add law tests for ContravariantMonoidal? Then I think we're good to go.
There was a problem hiding this comment.
Sure, I'll get to that today / tomorrow probably.
There was a problem hiding this comment.
I did it with kind of stupid Eq instances, but I'm not sure there are better ones. If there's a standard way to do Eq for functions, let me know and I can change it up.
|
Thanks @stephen-lazaro! |
Alias existing functions from symbolic ones
Add contravariant instance
Guarantee all instances serializable
For #202.