-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
SemigroupK/MonoidK methods #4084
Changes from 31 commits
0e0f0d4
9ea68b4
9f348f7
b0b1404
72fe714
9fb1bd1
e6fa448
c4a620e
0d7de79
9264e7c
5242b6c
93de0b8
c2d3d4f
7099f59
9c07dde
47232bf
4322e8b
05763b2
c237e17
2e5367a
bcc07d8
e350e70
fee0625
6b5f7fa
b531693
f5bdeca
7a0cd81
361b6c8
12053e1
a02f41e
19b3338
549a9f0
a744378
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,24 @@ | ||
package cats | ||
package data | ||
|
||
import Chain._ | ||
import Chain.{ | ||
empty, | ||
fromSeq, | ||
nil, | ||
one, | ||
sentinel, | ||
traverseFilterViaChain, | ||
traverseViaChain, | ||
Append, | ||
ChainIterator, | ||
ChainReverseIterator, | ||
Empty, | ||
NonEmpty, | ||
Singleton, | ||
Wrap | ||
} | ||
import cats.kernel.instances.StaticMethods | ||
import cats.kernel.compat.scalaVersionSpecific._ | ||
|
||
import scala.annotation.tailrec | ||
import scala.collection.immutable.{IndexedSeq => ImIndexedSeq, SortedMap, TreeSet} | ||
|
@@ -678,6 +694,7 @@ sealed abstract class Chain[+A] { | |
} | ||
} | ||
|
||
@suppressUnusedImportWarningForScalaVersionSpecific | ||
object Chain extends ChainInstances { | ||
|
||
private val sentinel: Function1[Any, Any] = new scala.runtime.AbstractFunction1[Any, Any] { def apply(a: Any) = this } | ||
|
@@ -1029,6 +1046,7 @@ sealed abstract private[data] class ChainInstances extends ChainInstances1 { | |
|
||
def empty[A]: Chain[A] = Chain.nil | ||
def combineK[A](c: Chain[A], c2: Chain[A]): Chain[A] = Chain.concat(c, c2) | ||
override def fromIterableOnce[A](xs: IterableOnce[A]): Chain[A] = Chain.fromSeq(xs.iterator.toSeq) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit... god... I will never stop finding micro issues: override def fromIterableOnce[A](xs: IterableOnce[A]): Chain[A] =
xs match {
case s: Seq[A @unchecked] =>
// Seq is a subclass of IterableOnce, so the type has to be compatible
Chain.fromSeq(s) // pay O(1) not O(N) cost
case notSeq =>
Chain.fromSeq(notSeq.iterator.toSeq)
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could it make sense to add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, we can put it in Chain. I don't view this as a side effect so much as we are lacking linear or affine types. What we want is to say this method takes ownership of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: It would be really cool if scala added ownership types but I think it is a bit complex to get right and ergonomic. |
||
def pure[A](a: A): Chain[A] = Chain.one(a) | ||
def flatMap[A, B](fa: Chain[A])(f: A => Chain[B]): Chain[B] = | ||
fa.flatMap(f) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package cats.instances | ||
|
||
import scala.collection.mutable.Builder | ||
|
||
private[instances] object instances { | ||
|
||
def appendAll[F <: Iterable[A], A](it: Iterator[F], bldr: Builder[A, F]): bldr.type = { | ||
while (it.hasNext) { | ||
bldr ++= it.next() | ||
} | ||
bldr | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have suggested making this final, but on second thought I think it should not be.
It could be that F is very efficient to build from a foldRight or a foldLeft or a foldMap. Since we should assume that the foldable for G knows the most efficient way to do those, the
F
implementation here may want to call one of those rather than converting to anIterable[A]
and going from there.This is probably minor, and maybe never a big deal, but the
final
will block this (I may have suggested or previously agreed with final, if so, apologies for the flip-flop).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, that's a good point.
FTR this was my suggestion in #4084 (comment).
The problem is we can't currently check this implementation in the laws; see #4084 (comment).
So there's a bit of performance vs yolo factor here 😉
We can always take away a
final
modifier, but we can't add it back. So I'd be fine to leave this until a practical need to change this comes up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't we check the laws? why can't we check that
fromFoldable(g) == fromIterable(g.toIterable)
in the laws?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah AFAICT we would have to add extra type parameters to
AlternativeTests#alternative
to do this. Unless I've missed something?