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

Add Endo type alias for (A =>A) #2076

Merged
merged 4 commits into from
Dec 10, 2017

Conversation

kailuowang
Copy link
Contributor

@kailuowang kailuowang commented Dec 6, 2017

Endo is necessary when I found that List[A => A]().foldK doesn't work, it has to use an Endo type alias
the main use case is

scala> import cats.implicits._, cats.Endo
scala> val l: List[Endo[Int]] = List(_ * 2, _ + 3)
scala> val f = l.reduceRightK //using `compose` to chain the functions
scala> f(1)
res0: Int = 5

Update: to further clarify, using reduceRightK, the functions inside the list is applied from left to right. If we use the existing foldK 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 adding Endo

@kailuowang kailuowang added this to the 1.0.0 milestone Dec 6, 2017
@codecov-io
Copy link

codecov-io commented Dec 6, 2017

Codecov Report

Merging #2076 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2076   +/-   ##
=======================================
  Coverage   94.66%   94.66%           
=======================================
  Files         318      318           
  Lines        5383     5383           
  Branches      207      207           
=======================================
  Hits         5096     5096           
  Misses        287      287
Impacted Files Coverage Δ
core/src/main/scala/cats/package.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/instances/function.scala 92.3% <ø> (ø) ⬆️
core/src/main/scala/cats/MonoidK.scala 80% <0%> (ø) ⬆️
core/src/main/scala/cats/SemigroupK.scala 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 084b984...828be44. Read the comment docs.

* res0: Int = 5
* }}}
*/
def reduceRightK[G[_], A](fga: F[G[A]])(implicit G: MonoidK[G]): G[A] =
Copy link
Contributor

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.

Copy link
Contributor Author

@kailuowang kailuowang Dec 7, 2017

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

Copy link
Contributor

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?

Copy link
Contributor Author

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?

LukaJCB
LukaJCB previously approved these changes Dec 7, 2017
@kailuowang kailuowang changed the title Add reduceRightK and Endo type alias for (A =>A) Add Endo type alias for (A =>A) Dec 7, 2017
@kailuowang kailuowang modified the milestones: 1.0.0, 1.1 Dec 7, 2017
ceedubs
ceedubs previously approved these changes Dec 7, 2017
Copy link
Contributor

@ceedubs ceedubs left a 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.

@kailuowang
Copy link
Contributor Author

merge with 2 sign-off.

@kailuowang kailuowang merged commit 3e1a053 into typelevel:master Dec 10, 2017
@kailuowang kailuowang deleted the add-reduceRightKAndEndo branch December 10, 2017 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants