Welcome, Alleycats - #1984
Conversation
|
not sure if anyone wants to review all the alleycats code. I think only the |
| import org.typelevel.discipline.Laws | ||
|
|
||
|
|
||
| trait FlatMapRecTests[F[_]] extends Laws { |
There was a problem hiding this comment.
I believe this is the only way to test the one specific rule out of the rule set, in this case tailRecMConsistentFlatMap
|
|
||
| // based upon foldRight of List in Cats | ||
| override def foldRight[A, B](fa: Iterable[A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] = { | ||
| def loop(as: Iterable[A]): Eval[B] = |
There was a problem hiding this comment.
I think this can actually be faster and get code-reuse if we use Foldable.iterateRight: https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/Foldable.scala#L508
| object IterableInstances { | ||
| @export(Orphan) | ||
| implicit val exportIterableFoldable: Foldable[Iterable] = | ||
| new Foldable[Iterable] { |
There was a problem hiding this comment.
I don't actually know why this shouldn't be in cats-core. What is the problem exactly? Foldable is lawless to begin with.
There was a problem hiding this comment.
AFAIK Iterable's only method is iterator which isn't exactly referentially transparent, no?
There was a problem hiding this comment.
But that is the only method you need to implement Foldable. And I don’t see why creating a new Iterator is not referentially transparent. It certainly is for all instances of Iterable is the standard library AFAIK.
There was a problem hiding this comment.
I don't know if all Iterable are actually Foldable, what about Stream?
There was a problem hiding this comment.
Doesn’t Stream have a Foldable instance in cats already? What prevents it from being Foldable?
There was a problem hiding this comment.
you are right, Stream is Foldable. Yeah, I don't see why we couldn't move Foldable ofIterable into core.
| trait FoldableSyntax { | ||
| implicit class ExtraFoldableOps[F[_]: Foldable, A](fa: F[A]) { | ||
| def foreach(f: A => Unit): Unit = | ||
| fa.foldLeft(()) { (unit, a) => f(a); unit } |
There was a problem hiding this comment.
why aren't we returning f(a) here? I don't understand? It is Unit right?
| def foldLeft[A, B](fa: Set[A], b: B)(f: (B, A) => B): B = | ||
| fa.foldLeft(b)(f) | ||
| def foldRight[A, B](fa: Set[A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] = | ||
| Foldable.iterateRight(fa.iterator, lb)(f) |
There was a problem hiding this comment.
This one needs to be
Foldable.iterateRight(fa, lb)(f)
28b845b to
a6e85dd
Compare
|
👍 Better late than never 😛 |
|
👍 |
4 participants
This PR moves alleycats into cats. We had some back and forth on this issue, but more and more evidences show it's more beneficial to have it inside cats repo.
It still has its own root package as
alleycatsand is only available when user explicitly add it as a dependency. So that users shall be careful when using instances defined here.In the process, I removed the dependencies from alleycats to algebra, since 1) this should be solely for outlaw TCs in cats and 2) algebra depends on cats.kernel.
I believe it is fine since kittens (which I co-maintain) is the only project that uses alleycats and it doesn't use the part in alleycats that has those dependencies.
The immediate incentive is for us to move traverse and foldable instances for map and set in here.