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

Fix Foldabe#dropWhile_ bug. #623

Merged
merged 3 commits into from
Nov 13, 2015
Merged

Fix Foldabe#dropWhile_ bug. #623

merged 3 commits into from
Nov 13, 2015

Conversation

non
Copy link
Contributor

@non non commented Nov 11, 2015

In addition to fixing this bug, the commit defines several
FoldableCheck tests for various types that have foldable
instances.

Finally, the commit adds takeWhile_.

Fixes #563.

In addition to fixing this bug, the commit defines several
FoldableCheck tests for various types that have foldable
instances.

Finally, the commit adds takeWhile_.

Fixes #563.
@codecov-io
Copy link

Current coverage is 76.35%

Merging #623 into master will increase coverage by +0.22% as of 3ca7a2b

@@            master    #623   diff @@
======================================
  Files          159     159       
  Stmts         2070    2081    +11
  Branches        69      70     +1
  Methods          0       0       
======================================
+ Hit           1576    1589    +13
  Partial          0       0       
+ Missed         494     492     -2

Review entire Coverage Diff as of 3ca7a2b

Powered by Codecov. Updated on successful CI builds.

package std

trait IterableInstances {
implicit val iterableInstance: Foldable[Iterable] =
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this belong in cats core? In a recent gitter conversation, the consensus was to put IndexedSeq instances in alleycats because IndexedSeq can potentially be mutable. Wouldn't the same arguments apply to Iterable?

@ceedubs
Copy link
Contributor

ceedubs commented Nov 11, 2015

You didn't mention that this PR also adds a Foldable[Iterable] instance :). I've left a question about that. Other than that, these changes look great to me.

@non
Copy link
Contributor Author

non commented Nov 11, 2015

My claim is that foldable instances are different (e.g. we have one for Set), and none of the foldable methods return F[A]. But I'm fine removing the instance too, it just seemed to me to be an oversight.

@ceedubs
Copy link
Contributor

ceedubs commented Nov 11, 2015

@tpolecat what are your thoughts?

Are we checking FoldableLaws for the Foldable[Iterable]? It doesn't look like it to me, but I might be overlooking some inheritance or something.

@tpolecat
Copy link
Member

Bad Cop to the rescue! So, yeah I think Foldable[Iterable] is bogus because the internal call to .iterator is observably side-effecting. That is,

val a = F.fold(xs)
val b = F.fold(xs)

is not necessarily the same program as

val a = F.fold(xs)
val b = a

even though fold is called with a pure value.

@non
Copy link
Contributor Author

non commented Nov 12, 2015

Ugh. OK, sure.

Personally I'd prefer to use correctness-preserving when dealing with something like Scala-collections but I see your point. I'll remove it. 🔨 🔥

Iterable is one of the most useful Scala collection
types. It's also impossible to be sure that its
.iterator method doesn't side-effect. C'est la vie.
@ceedubs
Copy link
Contributor

ceedubs commented Nov 12, 2015

👍

1 similar comment
@adelbertc
Copy link
Contributor

👍

adelbertc added a commit that referenced this pull request Nov 13, 2015
@adelbertc adelbertc merged commit 692e855 into master Nov 13, 2015
@non non deleted the topic/foldable-check branch April 28, 2016 03:52
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.

Foldable.dropWhile_() is not working as advertised
6 participants