Add SemigroupK and MonoidK laws (address #196). #198
Conversation
|
👍 |
|
Thanks! Unfortunately I'm not sure #196 was right. Algebra already has: https://github.com/non/algebra/blob/master/laws/src/main/scala/algebra/laws/GroupLaws.scala#L25 and: https://github.com/non/algebra/blob/master/laws/src/main/scala/algebra/laws/GroupLaws.scala#L52 |
|
Should we add SemigroupK/MonoidK laws instead?
|
|
Yes, that's a great idea! |
|
Ok. I will refactor them :)
|
lvicentesanchez
changed the title
|
Looks good to me! 👍 |
| new MonoidKTests[F, A] { | ||
| def EqFA = eqfa | ||
| def ArbA = implicitly[Arbitrary[A]] | ||
| def ArbF = implicitly[ArbitraryK[F]] |
There was a problem hiding this comment.
This is a totally minor thing, but could you please make use the implicit parameter list for the above two implicit instances like you have for eqfa? This would make it consistent with the style convention that was decided upon in #27 (but that has yet to be properly documented).
|
Two minor stylistic comments. Once those are addressed, this looks good to go! |
|
👍 thanks! |
|
Looks good! 👍 This PR needs to be updated before it can be merged (maybe due to a conflict) but as soon as that happens I'm happy to merge it. |
|
I will update it as soon as I get home.
|
|
I have just fixed the conflicts and updated the MonoidK and SemigroupK tests to be consistent with latest changes. |
| @@ -20,4 +20,7 @@ class StdTests extends FunSuite with Discipline { | |||
| checkAll("Option[Int]", MonadFilterTests[Option].monadFilter[Int, Int, Int]) | |||
| checkAll("Option[String]", MonadFilterTests[Option].monadFilter[String, Int, Int]) | |||
| checkAll("List[Int]", MonadFilterTests[List].monadFilter[Int, Int, Int]) | |||
| checkAll("List[Int]", MonoidKTests[List].identity[Int]) | |||
| checkAll("Stream[Int]", MonoidKTests[Stream].identity[Int]) | |||
| checkAll("Vector[Int]", MonoidKTests[Vector].identity[Int]) | |||
There was a problem hiding this comment.
Is .identity the right thing here? I would think it would be something like .monoidK, which would test all MonoidK laws, including those inherited from SemigroupK.
There was a problem hiding this comment.
Yes, that's a better name. I will update it.
On 19 Feb 2015 23:50, "Cody Allen" notifications@github.com wrote:
In tests/src/test/scala/cats/tests/StdTests.scala
https://github.com/non/cats/pull/198#discussion_r25038347:@@ -20,4 +20,7 @@ class StdTests extends FunSuite with Discipline {
checkAll("Option[Int]", MonadFilterTests[Option].monadFilter[Int, Int, Int])
checkAll("Option[String]", MonadFilterTests[Option].monadFilter[String, Int, Int])
checkAll("List[Int]", MonadFilterTests[List].monadFilter[Int, Int, Int])
- checkAll("List[Int]", MonoidKTests[List].identity[Int])
- checkAll("Stream[Int]", MonoidKTests[Stream].identity[Int])
- checkAll("Vector[Int]", MonoidKTests[Vector].identity[Int])
Is .identity the right thing here? I would think it would be something
like .monoidK, which would test all MonoidK laws, including those
inherited from SemigroupK.Reply to this email directly or view it on GitHub
https://github.com/non/cats/pull/198/files#r25038347.
|
👍 |
1 similar comment
|
👍 |
Add SemigroupK and MonoidK laws (address #196).
This PR addresses issue #196 by adding both laws and tests for Semigroup and Monoid typeclasses.