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
sequenceK semigroup combine fix #922
sequenceK semigroup combine fix #922
Conversation
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.
Could you please add the MonoidLaws test at least for SequenceKTests in this same diff? It's not a blocker but I'd rather have it now than later :)
da23615
to
44d1caf
Compare
@pakoito yeh, I started adding tests but had some problems with generators at first. added to PR for sequenceK now |
44d1caf
to
48c8d88
Compare
TraverseLaws.laws(this, this, { n: Int -> SequenceK(sequenceOf(n)) }, eq), | ||
SemigroupKLaws.laws(this, this, eq), | ||
MonoidLaws.laws(SequenceK.monoid(), Gen.list(Gen.int()).map{it.asSequence()}.generate().k(), eq), | ||
SemigroupLaws.laws(SequenceK.semigroup(), |
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.
These are not needed, as MonoidLaws includes SemigroupLaws.
@@ -32,7 +34,14 @@ class SequenceKTest : UnitSpec() { | |||
ShowLaws.laws(show, eq) { sequenceOf(it).k() }, | |||
MonadLaws.laws(this, eq), | |||
MonoidKLaws.laws(this, this, eq), | |||
TraverseLaws.laws(this, this, { n: Int -> SequenceK(sequenceOf(n)) }, eq) | |||
TraverseLaws.laws(this, this, { n: Int -> SequenceK(sequenceOf(n)) }, eq), | |||
SemigroupKLaws.laws(this, this, eq), |
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.
These are not needed, as MonoidKLaws includes SemigroupKLaws.
@richard-gibson one small fix and we're good to go! |
@pakoito no prob, do you mind if I also make SequenceKMonoidInstance inherit SequenceKSemigroupInstance so there is no duplication of combine? |
48c8d88
to
9656e64
Compare
@richard-gibson thanks for the fix!, merging this now, feel free to submit another one to avoid duplication of |
* sequenceK semigroup combine fix * Add missing closing parens
No description provided.