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

Added Reducible[_] type class. #298

Merged
merged 8 commits into from
May 21, 2015
Merged

Added Reducible[_] type class. #298

merged 8 commits into from
May 21, 2015

Conversation

non
Copy link
Contributor

@non non commented May 2, 2015

This commit does a lot of related things. Here's a list:

  1. Removes existing foldRight, renames foldLazy to foldRight
  2. Makes Fold[_] instances a bit more powerful to work with
  3. Removes Comonad[OneAnd[?, F]] instance.
  4. Add some documentation.
  5. Some whitespace and formatting changes in a few files.

These changes are all possibly contentious.

(1) was intended to address the confusing bestiary of various
fold methods: foldLeft, foldRight, and foldLazy. Scala's eager
foldRight method is mostly-useless, which is why it seemed
reasonable to just remove it and replace it with our version.
I would be open to a different name -- I became convinced
foldLazy was a bad name but maybe I'm wrong?

(2) adds the .imap and .complete methods to Fold[_]. In
particular, .complete is something that almost anyone working
with Fold[_] will probably want.

(3) addresses a problem I noticed awhile ago: the Comonad
instance we had for OneAnd only dealt with the .head, dropping
the .tail on the floor. This is definitely wrong, although
we don't have a law to encode this yet. I noticed it while
working to have OneAnd implement Reducible.

(4) just adds a bit of documentation that I felt was missing.

(5) is arguably bad, but I tried to clean up some files that
I happened to be editing. I am torn between wanting to leave
lines alone (so that git blame is more useful and to preserve
the original author's intentions) and standardizing on a more
approachable style.

This commit does a lot of related things. Here's a list:

 1. Removes existing foldRight, renames foldLazy to foldRight
 2. Makes Fold[_] instances a bit more powerful to work with
 3. Removes Comonad[OneAnd[?, F]] instance.
 4. Add some documentation.
 5. Some whitespace and formatting changes in a few files.

These changes are all possibly contentious.

(1) was intended to address the confusing bestiary of various
fold methods: foldLeft, foldRight, and foldLazy. Scala's eager
foldRight method is mostly-useless, which is why it seemed
reasonable to just remove it and replace it with our version.
I would be open to a different name -- I became convinced
foldLazy was a bad name but maybe I'm wrong?

(2) adds the .imap and .complete methods to Fold[_]. In
particular, .complete is something that almost anyone working
with Fold[_] will probably want.

(3) addresses a problem I noticed awhile ago: the Comonad
instance we had for OneAnd only dealt with the .head, dropping
the .tail on the floor. This is definitely wrong, although
we don't have a law to encode this yet. I noticed it while
working to have OneAnd implement Reducible.

(4) just adds a bit of documentation that I felt was missing.

(5) is arguably bad, but I tried to clean up some files that
I happened to be editing. I am torn between wanting to leave
lines alone (so that git blame is more useful and to preserve
the original author's intentions) and standardizing on a more
approachable style.
@non non added the in progress label May 2, 2015
@@ -0,0 +1,186 @@
package cats
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you might have accidentally added this back to git. It has been replaced by Xor.

@ceedubs
Copy link
Contributor

ceedubs commented May 5, 2015

I left a comment about naming on the issue. I'm okay with this being named Reducible, but I thought I would just bring up the thoughts I had.

At some point, scalaz added foldMapRight1 (which would probably be reduceMapRight for Cats) as an abstract method on Foldable1. This allows for optimizations in certain cases like Foldable1.toNel and may be worth considering. @non is quite good about optimization, so it's possible he is already doing something efficient that I'm overlooking.

I think UnsafeFoldable should be documented/justified since it's unsafe.

There's a lot to digest in this PR and I'm not sure I absorbed all of it. Other than Or.scala being added back in, and UnsafeFoldable being documented/justified, I think I'm okay with this getting merged and possible future refinements coming later. Also a rebase is needed.

@non
Copy link
Contributor Author

non commented May 5, 2015

@ceedubs Good catch with Or -- I'll fix it.

I'll take a look at what Scalaz is doing. In principle this should be doing the same thing but I want to be sure.

non added 2 commits May 4, 2015 23:49
1. Remove the "unsafe" Reducible class
2. Rewrite Reducible in terms of reduceLeftTo and reduceRightTo
3. Improve the documentation a bit

There was no need in Cats itself for #1. It seemed better to not
provide a dangerous type unless we had an active need. As far as
.toNEL, which @ceedubs mentioned.

As far as #3, the docs can always be better! ;)
@non
Copy link
Contributor Author

non commented May 11, 2015

@ceedubs I took your comments to heart. reduceLeftTo and reduceRightTo are the methods that would do this kind of work. I also removed the unsafe classes.

* Convert F[A] to a List[A], dropping all initial elements which
* match `p`.
*/
def dropWhile_[A](fa: F[A])(p: A => Boolean): List[A] =
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning/convention behind the underscores here and in filter_?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I took the naming convention from other places in Foldable, where you have less powerful versions of sequence and traverse which are named sequence_ and traverse_ (as opposed to the methods named in Traversable).

Similarly, in this case we can't filter or dropWhile to an F[A] value, but only to a List[A] (unlike something like MonadFilter). So I decided to use the same convention.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that makes sense. If I have this question, I imagine others will too. I wonder if there's a good way to document it and make it easy to discover.

@ceedubs
Copy link
Contributor

ceedubs commented May 20, 2015

@non sorry it took me so long to get around to a review. I think this PR does a lot of great things, and I like the approaches it takes. I've left some questions, but once they are addressed, I plan to give this a thumbs-up.

Improved some comments, added a final, and reinstated
Comonad[NonEmptyList].
@ceedubs
Copy link
Contributor

ceedubs commented May 20, 2015

👍

1 similar comment
@stew
Copy link
Contributor

stew commented May 21, 2015

👍

stew added a commit that referenced this pull request May 21, 2015
Added Reducible[_] type class.
@stew stew merged commit 2e22999 into master May 21, 2015
@stew stew removed the in progress label May 21, 2015
@non non deleted the topic/add-reducible branch June 21, 2015 15:55
@adelbertc adelbertc mentioned this pull request Nov 2, 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.

3 participants