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

Switch from generating scala.collection.Seq to scala.collection.immutable.Seq #150

Merged
merged 1 commit into from
Sep 7, 2019

Conversation

smarter
Copy link
Contributor

@smarter smarter commented Aug 5, 2019

No effect on 2.12, but on 2.13 this is what should be preferred and
makes the most sense since we're dealing with immutable values.

@eed3si9n
Copy link
Member

eed3si9n commented Aug 5, 2019

sbt-buildinfo has been using scala.collection.Seq since 2016 (#81) long before the Scala 2.13 migration happened, and given that the generated code might be part of someone else's API, the fact that it's scala.collection.Seq and not scala.Seq is a happy coincidence that would allow building without changing the semantics between the cross-Scala builds.

scala.collection.Seq is the first option we recommend in the migration guide as well.

@smarter
Copy link
Contributor Author

smarter commented Aug 6, 2019

But scala.Seq (or scala.collection.immutable.Seq if you prefer) is really what you want for immutable things, and what you want by default (after all, that's the whole reason why scala.Seq is now scala.collection.immutable.Seq, because it's a better default).

scala.collection.Seq is the first option we recommend in the migration guide as well.

I don't think that switching a codebase to scala.collection.Seq is realistic, it's fine in a few cases, but to use it consistently in a codebase requires a huge refactoring and means fighting against the spirit of 2.13 (varargs, every library you're using which uses scala.Seq, ...).

On the other hand, when moving from 2.12 to 2.13, I didn't have to do that many changes in my code, but I had to add a few .toSeq for values coming from sbt-buildinfo, this wastes CPU resources for no reason.

@eed3si9n
Copy link
Member

eed3si9n commented Aug 6, 2019

What do you think about using concrete datatypes like Vector instead so it would be immutable for both pre-2.13 and 2.13+?

@smarter
Copy link
Contributor Author

smarter commented Aug 6, 2019

That's also fine

This seems like the most reasonable choice and works better with the
default scala.Seq in 2.13.
@smarter
Copy link
Contributor Author

smarter commented Sep 7, 2019

I switched to scala.collection.immutable.Seq since that seems like the most natural choice, but happy to replace that with Vector if you prefer.

@eed3si9n eed3si9n merged commit 36a0c14 into sbt:master Sep 7, 2019
@eed3si9n eed3si9n changed the title Switch from generating scala.collection.Seq to scala.Seq Switch from generating scala.collection.Seq to scala.collection.immutable.Seq Aug 8, 2020
@eed3si9n eed3si9n added this to the 0.10.0 milestone Aug 8, 2020
bjaglin pushed a commit to scala-steward/scalafix that referenced this pull request Oct 2, 2020
Ignore Seq signature changes coming from the breaking change
sbt/sbt-buildinfo#150
bjaglin pushed a commit to scala-steward/scalafix that referenced this pull request Oct 2, 2020
Ignore Seq signature changes coming from the breaking change
sbt/sbt-buildinfo#150
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

Successfully merging this pull request may close these issues.

2 participants