Fixed #1411 - updated Chunk to use scala.collection.Seq #1413
Conversation
| @@ -534,16 +535,16 @@ object Chunk { | |||
| override def map[O2](f: O => O2): Chunk[O2] = indexedSeq(s.map(f)) | |||
| } | |||
|
|
|||
| /** Creates a chunk backed by a `Seq`. */ | |||
| def seq[O](s: Seq[O]): Chunk[O] = s match { | |||
| /** Creates a chunk backed by a mutable `Seq`. */ | |||
There was a problem hiding this comment.
A collection.Seq an be either mutable or immutable, right?
There was a problem hiding this comment.
Correct. It is a parent of both collection.immutable.Seq and collection.mutable.Seq.
There was a problem hiding this comment.
This is not binary compatible on 2.13.0-M5, right? Is the strategy to wait until the next 2.13.0, or to say that's not subject to binary compatibility, being a standard library milestone? I'm content either way.
👍 to the spirit of the change. collection.Seq is not my favorite type, but we'll see it a lot until Scala 2.12 falls into disuse.
| case a: collection.mutable.WrappedArray[O] => | ||
| array(a.array.asInstanceOf[Array[O]]) | ||
| case v: Vector[O] => vector(v) | ||
| case ix: IndexedSeq[O] => indexedSeq(ix) | ||
| case _ => | ||
| if (s.isEmpty) empty | ||
| else if (s.tail.isEmpty) singleton(s.head) | ||
| else buffer(collection.mutable.Buffer(s: _*)) | ||
| else buffer(collection.mutable.Buffer((s.toSeq): _*)) |
There was a problem hiding this comment.
Would a match for Buffer to go through buffer without s.toSeq be worthwhile? I doubt this gets passed Buffers often, but it's what jawn-fs2 always sends. I may end up special casing it there, even after this fix.
There was a problem hiding this comment.
Probably a good idea.
The s.toSeq here is weird -- I think that will no longer be necessary as of 2.13.0-RC1. I'll add a comment to double check that.
I was thinking of saying no binary compatibility on 2.13 milestones.
Same :) |
|
Okay this one should be ready for re-review & merge. |
| @@ -249,6 +249,13 @@ lazy val core = crossProject(JVMPlatform, JSPlatform) | |||
| .settings( | |||
| name := "fs2-core", | |||
| sourceDirectories in (Compile, scalafmt) += baseDirectory.value / "../shared/src/main/scala", | |||
| unmanagedSourceDirectories in Compile += { | |||
| val dir = CrossVersion.partialVersion(scalaVersion.value) match { | |||
There was a problem hiding this comment.
Doesn't sbt support this just by naming the source directory scala-2.13- (or something like that)? I'm looking at cats: https://github.com/typelevel/cats/tree/master/core/src/main
There was a problem hiding this comment.
That appears to be done in build.sbt here:
https://github.com/typelevel/cats/blob/master/build.sbt#L21
There was a problem hiding this comment.
So playing around with IndexedSeq I've noticed that it has the same change from 2.12 -> 2.13 i.e. becomes immutable. I'm wondering whether you need to apply the same import collection.IndexedSeq to make this work with both types. Specifically, is there a way to check that inside Chunk.seq (now Chunk.iterable) mutable IndexedSeqs are matching correctly?
| @@ -1,6 +1,7 @@ | |||
| package fs2 | |||
|
|
|||
| import scala.collection.immutable.{Queue => SQueue} | |||
| import scala.collection.{Seq => GSeq} | |||
There was a problem hiding this comment.
I've noticed that this problem also extends to IndexedSeq. Is it necessary to give it the same treatment?
| // cast is safe b/c the array constructor will check for primitive vs boxed arrays | ||
| array(arr.asInstanceOf[Array[O]]) | ||
| case v: Vector[O] => vector(v) | ||
| case ix: IndexedSeq[O] => indexedSeq(ix) |
There was a problem hiding this comment.
Suspect this is only picking up immutable IndexedSeqs, as explained above.
Thanks, fixed |
Initial pass at fixing #1411. One issue that remains is that varargs on 2.13 passes an
ArraySeqnow, not aWrappedArray, so I'm unsure if the optimization inChunk.seqis accomplishing anything. I think I looked in to this once before but will do so again.