-
Notifications
You must be signed in to change notification settings - Fork 453
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
#841 - Add Monoid instances #942
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.
Looks great so far, let us know when this is ready for final review, thanks!
interface EitherMonoidInstance<L, R> : EitherSemigroupInstance<L, R>, Monoid<Either<L, R>> { | ||
fun MO(): Monoid<R> | ||
|
||
override fun SG(): Semigroup<R> { |
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.
style nit pick, favor simple expressions whenever possible:
override fun SG(): Semigroup<R> = MO()
interface TryMonoidInstance<A> : TrySemigroupInstance<A>, Monoid<Try<A>> { | ||
fun MO(): Monoid<A> | ||
|
||
override fun SG(): Semigroup<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.
style nit pick, favor simple expressions whenever possible:
override fun SG(): Semigroup<A> = MO()
is Either.Left -> this | ||
is Either.Right -> { val x = this.b; when (b) { | ||
is Either.Left -> b | ||
is Either.Right -> Either.Right(SG().run { x.combine(b.b) }) |
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 this be done without the intermediate variable as this.b.combine(b.b)
?
fun SG(): Semigroup<A> | ||
|
||
override fun Try<A>.combine(b: Try<A>): Try<A> = | ||
when (this) { |
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'd move these functions to the datatype, reference them here instead.
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 a way to do that given the dependency on SG and the semigroup interface. Is there a good example to follow given that core doesn't import typeclasses to avoid a circular dependency?
@@ -1,11 +1,56 @@ | |||
package arrow.instances | |||
|
|||
import arrow.Kind | |||
import arrow.core.* |
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'm not sure why my editor is making that change, but I can revert if you'd like?
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.
Sure, although it isn't a blocker.
Codecov Report
@@ Coverage Diff @@
## master #942 +/- ##
===========================================
+ Coverage 45.42% 45.52% +0.1%
Complexity 647 647
===========================================
Files 309 309
Lines 7975 7997 +22
Branches 859 869 +10
===========================================
+ Hits 3623 3641 +18
Misses 4035 4035
- Partials 317 321 +4
Continue to review full report at Codecov.
|
add semigrouplaws tests add tests for combine
So far so good. Tell us when it's ready to review! |
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.
Great job!
Adding this as a WIP for now. Would there be more required for Try/Either?