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

Implement Reducible.reduceM #1193

Merged
merged 1 commit into from
Jul 15, 2016
Merged

Conversation

benhutchison
Copy link
Member

foldM for nonempty collections. First mention: https://gitter.im/typelevel/cats?at=5781d3f33eaf66535e62da69

@@ -60,6 +60,12 @@ import simulacrum.typeclass
def reduceLeftTo[A, B](fa: F[A])(f: A => B)(g: (B, A) => B): B

/**
* Monadic variant of `reduceLeftTo`
* */
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpicks:

Could we change the last line of the scaladoc entry to just */?

Also can we use [[reduceLeftTo]] so that the scaladoc has a handy link?

@ceedubs
Copy link
Contributor

ceedubs commented Jul 14, 2016

Thanks @benhutchison! It looks good -- I just left a few very minor formatting/stylistic thoughts.

I also wonder about using the name reduceLeftM and potentially adding a reduceLeftMRec once #1117 goes through, but those should probably wait until that PR is merged.

@codecov-io
Copy link

codecov-io commented Jul 14, 2016

Current coverage is 89.16%

Merging #1193 into master will increase coverage by <.01%

@@             master      #1193   diff @@
==========================================
  Files           234        234          
  Lines          3137       3139     +2   
  Methods        3083       3083          
  Messages          0          0          
  Branches         52         53     +1   
==========================================
+ Hits           2797       2799     +2   
  Misses          340        340          
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 5455ff1...1b954ed

@benhutchison
Copy link
Member Author

@ceedubs Updated.

I went ahead with name reduceLeftM because I think its clearer & already well considered in #1117. Avoids busywork later to change it.

@ceedubs
Copy link
Contributor

ceedubs commented Jul 14, 2016

👍 looks good to me once the build passes.

@@ -60,6 +60,12 @@ import simulacrum.typeclass
def reduceLeftTo[A, B](fa: F[A])(f: A => B)(g: (B, A) => B): B

/**
* Monadic variant of [[reduceLeftTo]]
*/
def reduceLeftM[G[_], A, B](fa: F[A])(f: A => G[B])(g: (B, A) => G[B])(implicit G: Monad[G]): G[B] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can change Monad to FlatMap here.

Copy link
Contributor

Choose a reason for hiding this comment

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

And since another change has been suggested I feel less guilty about this one: can you shift the asterisks one space to the left to be javadoc style like other comments in the file? :)

@peterneyens
Copy link
Collaborator

I left one minor comment, for the rest 👍

@ceedubs
Copy link
Contributor

ceedubs commented Jul 15, 2016

Nice work, @benhutchison!

@ceedubs ceedubs merged commit b304dfc into typelevel:master Jul 15, 2016
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