Explicitly use scala.collection.Seq to allow use of mutable collections in Scala 2.13 #176

tmccarthy · 2020-04-10T04:46:31Z

Since Scala 2.13, the universal Seq type alias has referred to scala.collection.immutable.Seq. Prior to this, it referred to scala.collection.Seq, which is the supertype for both mutable and immutable sequences (see migration note).

Since #121, this has caused an inconsistency in how plotly-scala handles Seq between the Scala 2.13 and 2.12 versions. In Scala 2.12, mutable collections like ArrayBuffer have an implicit conversion to plotly.Sequence. In Scala 2.13, no conversion exists, and clients are required to convert to immutable.Seq. An example of code that compiles in 2.12 but not 2.13 is included in this PR as a unit test.

This PR fixes this issue by explicitly importing scala.collection.Seq wherever it is used. In Scala 2.12 this is a no-op, since Seq is simply an alias for this type. In Scala 2.13 it restores the old behaviour from before #121.

Let me know if you have any questions or feedback!

Test fails for ./sbt tests/test, passes for ./sbt ++2.12.10! tests/test

Scala 2.12 aliases Seq to scala.collection.Seq, whereas Scala 2.13 aliases to scala.collection.immutable.Seq. Explicitly using scala.collection.Seq allows the use of both mutable and immutable sequences, and retains the behaviour before the Scala 2.13 migration.

alexarchambault · 2020-04-15T15:50:05Z

@tmccarthy Thanks for opening this!

AFAIC I've always tended to rely on immutable collections when using plotly-scala, so I didn't run into that problem.

I think using immutable collections may actually be more consistent. All classes in plotly-scala are already immutable, and have with* methods to change fields, that actually return new instances of the underlying class.

Do you think it would be possible to go the other way around in your uses of plotly-scala, and only pass immutable collections to it? Or is it really too cumbersome? My understanding is that calling .toSeq on mutable collections right before passing them to plotly-scala should be enough.

alexarchambault · 2020-04-15T15:56:17Z

Actually, the implicit conversion from mutable Seq to to plotly.Sequence can't hurt… I think I'd be ok merging the first commit of this PR.

tmccarthy · 2020-04-18T01:01:03Z

Thanks for the response Alex!

In Scala 2.12 and earlier, Seq referred to a supertype for both mutable and immutable collections. This type is what is used throughout plotly-scala, meaning that in 2.12, the project is broadly written against collections that can be mutable. This is not the case in Scala 2.13, where the Seq alias now refers to the supertype for immutable sequences only.

To be clear, I'm all in favour of targeting immutable collections. The .toSeq call is certainly not too cumbersome. My concern is that in plotly-scala 0.7.3, the Scala 2.12 artefact supports and uses potentially-mutable sequences but the 2.13 one doesn't.

Seeing as I'm the one that introduced this problem in #121, I thought I should fix it by restoring the previous behaviour 😅. That behaviour was to use the potentially-mutable Seq through the project.

I agree that it would be a nicer approach to make all sequences within plotly-scala immutable, and to provide an implicit conversion to plotly.Sequence for mutable collections. I'm happy to update this PR accordingly 👍.

…ons from mutable sequences to plotly.Sequence, with Scala-version-specific implementations.

…t once on the Sequence companion. Previous solution was over-engineered.

tmccarthy · 2020-04-18T06:43:06Z

core/shared/src/main/scala/plotly/Sequence.scala

+  private def makeImmutableSeq[A](mutableSeq: BaseScalaSeq[A]): Seq[A] =
+    mutableSeq match {
+      case asImmutableSeq: Seq[A] => asImmutableSeq
+      case _ => mutableSeq.toVector
+    }


This is basically equivalent to the .toSeq method in Scala 2.13. Since we are cross-compiling with Scala 2.12, we require a dedicated implementation here.

alexarchambault

Thanks! Just added a comment about keeping the default Seq by default (except in the Sequences conversions).

alexarchambault · 2020-04-19T10:13:39Z

almond/src/main/scala/plotly/Almond.scala

@@ -4,6 +4,7 @@ import java.lang.{Boolean => JBoolean, Double => JDouble, Integer => JInt}

 import almond.interpreter.api.{DisplayData, OutputHandler}

+import scala.collection.immutable.Seq


Let's keep relying on the default Seq. That makes the API slightly different in 2.12 and 2.13, but that these APIs accept Seq(…) in both cases, which is not the case in 2.12 if we require scala.collection.immutable.Seq explicitly.

alexarchambault · 2020-04-19T10:14:24Z

core/shared/src/main/scala/plotly/Trace.scala

@@ -1,5 +1,6 @@
 package plotly

+import scala.collection.immutable.Seq


Same as above, let's keep the default Seq, even though it's slightly different in 2.12 and 2.13, here and in all scala.collection.immutable.Seq added below.

…scala.collection.Seq` data type only in Scala 2.13 and up.

tmccarthy · 2020-04-19T12:26:22Z

If we're happy with the split between Seq implementations between 2.12 and 2.13, I think this is the easiest approach. I've simply provided a set of implicit conversions from scala.collection.Seq to plotly.Sequence in 2.13 only. This minimises the change footprint, but allows mutable collections to be converted to Sequence. Let me know if this hits the mark 🙂.

alexarchambault · 2020-04-19T14:27:18Z

That looks fine, thanks @tmccarthy!

Timothy McCarthy added 2 commits April 10, 2020 13:57

Add test for implicit conversion to Sequence which fails on Scala 2.13

5912627

Test fails for ./sbt tests/test, passes for ./sbt ++2.12.10! tests/test

Timothy McCarthy added 2 commits April 18, 2020 14:41

Use immutable sequences throughout project. Provide implicit conversi…

dcce28b

…ons from mutable sequences to plotly.Sequence, with Scala-version-specific implementations.

Remove Scala-version-specific mutable Sequence conversions, just do i…

a2b97d6

…t once on the Sequence companion. Previous solution was over-engineered.

tmccarthy commented Apr 18, 2020

View reviewed changes

alexarchambault reviewed Apr 19, 2020

View reviewed changes

Go back to using Seq alias. Provide implicit conversions from the `…

cea4c9d

…scala.collection.Seq` data type only in Scala 2.13 and up.

tmccarthy marked this pull request as draft April 19, 2020 12:19

Timothy McCarthy added 2 commits April 19, 2020 22:22

Fix version-specific directory naming

154dc75

Remove unneccessary whitespace

ad8e49a

tmccarthy marked this pull request as ready for review April 19, 2020 12:23

alexarchambault merged commit a9ea8c3 into alexarchambault:master Apr 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Explicitly use scala.collection.Seq to allow use of mutable collections in Scala 2.13 #176

Explicitly use scala.collection.Seq to allow use of mutable collections in Scala 2.13 #176

Uh oh!

tmccarthy commented Apr 10, 2020 •

edited

Loading

Uh oh!

alexarchambault commented Apr 15, 2020 •

edited

Loading

Uh oh!

alexarchambault commented Apr 15, 2020

Uh oh!

tmccarthy commented Apr 18, 2020 •

edited

Loading

Uh oh!

tmccarthy Apr 18, 2020

Uh oh!

alexarchambault left a comment

Uh oh!

alexarchambault Apr 19, 2020

Uh oh!

alexarchambault Apr 19, 2020

Uh oh!

tmccarthy commented Apr 19, 2020

Uh oh!

alexarchambault commented Apr 19, 2020

Uh oh!

Uh oh!

		@@ -4,6 +4,7 @@ import java.lang.{Boolean => JBoolean, Double => JDouble, Integer => JInt}

		import almond.interpreter.api.{DisplayData, OutputHandler}

		import scala.collection.immutable.Seq

		@@ -1,5 +1,6 @@
		package plotly

		import scala.collection.immutable.Seq

Explicitly use scala.collection.Seq to allow use of mutable collections in Scala 2.13 #176

Explicitly use scala.collection.Seq to allow use of mutable collections in Scala 2.13 #176

Uh oh!

Conversation

tmccarthy commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexarchambault commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexarchambault commented Apr 15, 2020

Uh oh!

tmccarthy commented Apr 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmccarthy Apr 18, 2020

Choose a reason for hiding this comment

Uh oh!

alexarchambault left a comment

Choose a reason for hiding this comment

Uh oh!

alexarchambault Apr 19, 2020

Choose a reason for hiding this comment

Uh oh!

alexarchambault Apr 19, 2020

Choose a reason for hiding this comment

Uh oh!

tmccarthy commented Apr 19, 2020

Uh oh!

alexarchambault commented Apr 19, 2020

Uh oh!

Uh oh!

tmccarthy commented Apr 10, 2020 •

edited

Loading

alexarchambault commented Apr 15, 2020 •

edited

Loading

tmccarthy commented Apr 18, 2020 •

edited

Loading