-
-
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 Endo
type alias for (A =>A
)
#2076
Add Endo
type alias for (A =>A
)
#2076
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2076 +/- ##
=======================================
Coverage 94.66% 94.66%
=======================================
Files 318 318
Lines 5383 5383
Branches 207 207
=======================================
Hits 5096 5096
Misses 287 287
Continue to review full report at Codecov.
|
* res0: Int = 5 | ||
* }}} | ||
*/ | ||
def reduceRightK[G[_], A](fga: F[G[A]])(implicit G: MonoidK[G]): G[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.
seems like combineAllK
to me.
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.
combineAllK
is foldK
right? This one combines in the opposite direction
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 see. I misunderstood.
I am very -1 on this unless I am mistaken. reduceRight(Option) is right associative, but does not reverse the effective order that we iterate through. This method does by swapping.
I would call this reverseCombineAllK
.
Since the MonoidK
is associative, the result is the same if you right associate or left associate (though there may be efficiency reasons to pick one). This is something else: it is like making a new Monoid that has the property that combine(a, b) == original.combine(b, a)
This is actually still a monoid, but it behaves differently for non-commutative cases.
@kailuowang can you explain if I am wrong about this being different from reduceRight?
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.
You are right it's different from reduceRight(Option). reverseCombineAllK
is probably a better name, but you also got to the bottom of it, we don't need it, we just need a different Monoid
instance that does the original.combine(b, a)
. Maybe adding a reverse
method to Monoid
and MonoidK
. But I am not sure are they worthwhile to be added?
What about the Endo
type alias?
reduceRightK
and Endo
type alias for (A =>A
)Endo
type alias for (A =>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.
An Endo
alias seems reasonable to me. I do also find myself wondering if there a more general solution to this problem. I tried out a couple of things locally but didn't have much luck.
merge with 2 sign-off. |
Endo
is necessary when I found thatList[A => A]().foldK
doesn't work, it has to use anEndo
type aliasthe main use case is
Update: to further clarify, usingreduceRightK
, the functions inside the list is applied from left to right. If we use the existingfoldK
the functions inside the list would be applied from right to left which gives a final result of 8.further Update:
reduceRightK
is removed. This PR is only about addingEndo