-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Make kernel laws consistent with core laws #1922
Make kernel laws consistent with core laws #1922
Conversation
d66e8a7
to
29f9434
Compare
Codecov Report
@@ Coverage Diff @@
## master #1922 +/- ##
==========================================
+ Coverage 95.6% 96.13% +0.53%
==========================================
Files 252 273 +21
Lines 4570 4530 -40
Branches 124 116 -8
==========================================
- Hits 4369 4355 -14
+ Misses 201 175 -26
Continue to review full report at Codecov.
|
7785850
to
0616d9b
Compare
I moved most of the machinery from Otherwise I'm pretty much done with this :) |
note algebra-laws do depend on this, so we will need to fix algebra before it can be updated after this is published: https://github.com/typelevel/algebra/blob/master/build.sbt#L143 |
I'll create a PR :) |
def laws: BoundedSemilatticeLaws[A] | ||
|
||
def boundedSemilattice(implicit arbA: Arbitrary[A], eqA: Eq[A]): RuleSet = | ||
new RuleSet { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: most of these new RuleSet {
where bases
is empty could be just new DefaultRuleSet(
right like the one you used in BandTests
? No big deal though, just that it could be slightly more concise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't work for Typeclasses with more than one parent though, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize that. disregard my comment then. Maybe we can create one that does in discipline
later.
import org.scalacheck.{Arbitrary, Prop} | ||
import cats.kernel.instances.boolean._ | ||
|
||
object Rules { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setup is indeed less readable than the cats.core way.
@@ -1,7 +1,7 @@ | |||
package cats | |||
package tests | |||
|
|||
import cats.kernel.laws.GroupLaws | |||
import cats.kernel.laws.discipline.{MonoidTests => MonoidTypeclassTests} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another nit: Maybe we can call these MonoidLawTests
, because the other MonoidTest
is also testing the Type Class.
|
||
import cats.kernel.Monoid | ||
|
||
trait MonoidLaws[A] extends SemigroupLaws[A] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find the replacement for Rules.isId("isEmpty", A.empty)(A.isEmpty)
?
new RuleSet { | ||
val name: String = "boundedSemilattice" | ||
val bases: Seq[(String, RuleSet)] = Nil | ||
val parents: Seq[RuleSet] = Seq(commutativeSemigroup, band) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be communtativeMonoid, semilattice
instead?
55c82a5
to
881553e
Compare
881553e
to
5073a95
Compare
override implicit def E: PartialOrder[A] | ||
|
||
def reflexitivity(x: A): IsEq[Boolean] = | ||
E.gteqv(x, x) <-> true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the original test uses ?<=
, i.e. E.lteqv
, I am not sure if it matters. (mabye we can have both lteqv
and gteqv
?)
15995be
to
d3464b1
Compare
@@ -45,7 +44,6 @@ class FunctionTests extends CatsSuite { | |||
checkAll("Function0[Eqed]", EqLawTests[Function0[Eqed]].eqv) | |||
checkAll("Function0[POrd]", PartialOrderLawTests[Function0[POrd]].partialOrder) | |||
checkAll("Function0[Ord]", OrderLawTests[Function0[Ord]].order) | |||
checkAll("Function0[Hsh]", HashLaws[Function0[Hsh]].hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, was this test removed intentionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I removed it initially for erroring for erroring out, as with the new setup, all the laws are tested and you can't single out one specific law to run. I wanted to replace it with a manual test, but forgot, will fix :)
d3464b1
to
8dbbb81
Compare
I added another MiMa exception as I had to reshuffle the option instances :) |
@@ -54,6 +55,13 @@ class FunctionTests extends CatsSuite { | |||
checkAll("Function0[Grp]", GroupLawTests[Function0[Grp]].group) | |||
checkAll("Function0[CGrp]", CommutativeGroupTests[Function0[CGrp]].commutativeGroup) | |||
|
|||
test("Function0[Hsh]") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious why not just test the hash law? won't pass?
checkAll("Function0[Grp]", GroupLawTests[Function0[Grp]].group) | ||
checkAll("Function0[CGrp]", CommutativeGroupTests[Function0[CGrp]].commutativeGroup) | ||
|
||
test("Function0[Hsh]") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, why not just test the full HashLaw? won't pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's not the same as universal hash and not the same as scala.util.Hashing
@@ -3,17 +3,19 @@ package instances | |||
|
|||
package object option extends OptionInstances | |||
|
|||
trait OptionInstances extends OptionInstances1 { | |||
trait OptionInstances extends OptionInstances2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we are reshuffling the number here, how about the we fix the priority number in one go? should be in the ascending order. BTW, @johnynek is this break okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed the numbers :) Btw, I think this break is an oversight from the Hash PR, as PartialOrder
and Hash
instances had the same priority and both extend Eq
, so it led to ambiguous implicits when requiring Eq
for Option
|
2a33438
to
d82b447
Compare
Thanks @kailuowang! Fixed it :) |
d82b447
to
fb163f3
Compare
fb163f3
to
1ca0ecd
Compare
sorry, more merge needed. |
No worries, done :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this huge job. This is a great service to cats and I appreciate it.
* Object with a dynamic variable that allows users to skip the | ||
* serialization tests for certain instances. | ||
*/ | ||
private[laws] object IsSerializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we lose this test entirely?
It is very important for spark and scalding (and others).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's replaced by the SerializableLaws
moved from cats.tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Oh damn just saw this @LukaJCB - you the man! |
Should fix #1272, still a WIP, but compiles and runs so looking for some feedback on some of the decisions made when porting between styles :)