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

add "import scala.jdk.CollectionConverters" option in scalapb compiler-plugin? #598

Closed
xuwei-k opened this issue Jun 13, 2019 · 5 comments
Closed

Comments

@xuwei-k
Copy link
Contributor

xuwei-k commented Jun 13, 2019

scala.collection.JavaConverters deprecated since Scala 2.13.

I think we should add new option for which class to use scala.jdk.CollectionConverters or scala.collection.JavaConverters for avoid deprecation warnings.

something like

  if (useJdkCollectionConverter) {
    printer.add("import scala.jdk.CollectionConverters._")
  } else {
    printer.add("import scala.collection.JavaConverters._")
  }

.when(javaConverterImport)(
_.add("import scala.collection.JavaConverters._").add()

@giiita
Copy link

giiita commented Jun 14, 2019

As a similar problem, default Seq has been changed from scala.collection.Seq to scala.collection.immutable.Seq from scala2.13'.
This will require a toSeq call during model conversion.

@xuwei-k
Copy link
Contributor Author

xuwei-k commented Jun 14, 2019

we should add new option

Alternatively, create custom compat converter object in scalapb-runtime 🤔

scala/scala-collection-compat#208

@thesamet
Copy link
Contributor

@xuwei-k yes! that would be my preference, so the same source compiles for all versions of Scala.

@giiita Can you clarify what problem you are referring to? Is there a warning you are seeing?

@giiita
Copy link

giiita commented Jun 14, 2019

@thesamet
No, there is no warning.

There is such an interface.

message AnyList {
    repeated string values = 1;
}
final case class AnyList(values: _root_.scala.collection.Seq[String])

In 2.12, the default Seq is scala.collection.Seq.

type Seq[+A] = scala.collection.Seq[A]
val Seq = scala.collection.Seq

So this worked.

case class MyList(values: Seq[String])

MyList(AnyList().values)

In contrast, in 2.13 it has been changed to scala.collection.immutable.Seq.

@migration("scala.Seq is now scala.collection.immutable.Seq instead of scala.collection.Seq", "2.13.0")
type Seq[+A] = scala.collection.immutable.Seq[A]
val Seq = scala.collection.immutable.Seq

so conversion by toSeq is necessary.

case class MyList(values: Seq[String])

MyList(AnyList().values.toSeq)

Since this breaks compatibility with 2.12, I think it is preferable to change the automatically generated code to immutable.Seq.

@thesamet
Copy link
Contributor

@giiita Both MyList.values and AnyList.values are scala.colleciton.Seq in both Scala 2.12 and Scala 2.13. Therefore, .toSeq is not necessary for either version of Scala. I might be missing something - if so, can you please start a separate issue so the discussion stays focused here stays focused on the JavaConverters warning?

thesamet added a commit that referenced this issue Jun 14, 2019
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

No branches or pull requests

3 participants