Skip to content
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

Adds Semigroup for Xor #717

Merged
merged 2 commits into from
Dec 7, 2015
Merged

Conversation

mikejcurry
Copy link
Contributor

As per #716, adding a Semigroup for the Xor data type which currently
has a Monoid instance but no Semigroup instance.

As per typelevel#716, adding a Semigroup for the Xor data type which currently
has a Monoid instance but no Semigroup instance.
@@ -12,7 +12,8 @@ import org.scalacheck.Arbitrary._
import scala.util.Try

class XorTests extends CatsSuite {
checkAll("Xor[String, Int]", GroupLaws[Xor[String, Int]].monoid)
checkAll("Monoid[Xor[String, Int]]", GroupLaws[Xor[String, Int]].monoid)
checkAll("Semigroup[Xor[String, NonEmptyList[Int]]]", GroupLaws[Xor[String, NonEmptyList[Int]]].semigroup)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: monoid/semigroup are already included in the test names, so I think the convention has been to not include Monoid/Semigroup in the string argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - I can certainly change this. For some reason I'd thought I'd seen a mix of these versions, but I see now it was likely my eyes deceiving me. :-)

@ceedubs
Copy link
Contributor

ceedubs commented Dec 6, 2015

👍 if/when the build passes.

@mikejcurry you are on a roll again! :)

@codecov-io
Copy link

Current coverage is 85.49%

Merging #717 into master will increase coverage by +0.02% as of 1660921

@@            master    #717   diff @@
======================================
  Files          162     162       
  Stmts         2237    2240     +3
  Branches        74      74       
  Methods          0       0       
======================================
+ Hit           1912    1915     +3
  Partial          0       0       
  Missed         325     325       

Review entire Coverage Diff as of 1660921

Powered by Codecov. Updated on successful CI builds.

@mikejcurry
Copy link
Contributor Author

Pushed a change to keep things in line with naming conventions for tests. Maybe I should add this convention to the contributors guide?

@ceedubs
Copy link
Contributor

ceedubs commented Dec 6, 2015

@mikejcurry that would be great! Thank you. I think the sole exception is the Cats serializability test, since the same one is used for all types. Actually, if you are already going to make a change, could you add that all type class instances should also be checked with the serializability tests unless they are being checked by Algebra's laws (GroupLaws, OrderLaws, etc), which already test serializability? This helps keep us Spark-friendly (and other tools that require java Serializability).

@mikejcurry
Copy link
Contributor Author

Sure, have to head away for a while now, so it may not be today, but I'll get to it some time soon.

@adelbertc
Copy link
Contributor

👍

adelbertc added a commit that referenced this pull request Dec 7, 2015
@adelbertc adelbertc merged commit bc07d85 into typelevel:master Dec 7, 2015
@mikejcurry mikejcurry deleted the xor-semigroup branch December 11, 2015 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants