-
-
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
Add SemigroupK and MonoidK laws (address #196). #198
Conversation
👍 |
Thanks! Unfortunately I'm not sure #196 was right. Algebra already has: and: |
Should we add SemigroupK/MonoidK laws instead?
|
Yes, that's a great idea! |
Ok. I will refactor them :)
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a better name. I will update it.
On 19 Feb 2015 23:50, "Cody Allen" [email protected] 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.