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 an iterator() alias to iterator #7

Closed
wants to merge 3 commits into from

Conversation

julienrf
Copy link
Contributor

@julienrf julienrf commented Mar 28, 2018

We already have a scalafix rule that translates .iterator calls to .iterator(). This rule can be applied to all the code bases to make them compatible with 2.13. However, once the rule has been applied the code won’t be compatible with 2.12 (or previous Scala version) anymore…

This PR introduces an iterator() extension method for Scala 2.12 so that after the application of the scalafix rule, the code can still compile with 2.12.

A second commit makes it possible to write newBuilder() instead of newBuilder, in 2.12.

Also, related discussion: scala/collection-strawman#520

@julienrf
Copy link
Contributor Author

I’m on the fence with the changes on parentheses. On one hand, there are some advantages of having a more disciplined used of parentheses (although, as noted by @Ichoran in the linked discussion, it is hard to find a really robust principle on when to use parentheses). But on the other hand that a source of incompatibilities between 2.13 and prior Scala versions… The trick I’m using here works for iterator() but it doesn’t always work for newBuilder(), because sometimes this newBuilder() method takes implicit parameters (typically, an Ordering or a ClassTag), and in that case the implicit conversion to the extension method is not triggered.

@@ -30,15 +30,31 @@ package object compat {
simpleCBF(fact.newBuilder)

implicit class IterableFactoryExtensionMethods[CC[X] <: GenTraversable[X]](private val fact: GenericCompanion[CC]) {
def from[A](source: TraversableOnce[A]): CC[A] = fact.apply(source.toSeq: _*)
def from[A](source: TraversableOnce[A]): CC[A] = (fact.newBuilder[A] ++= source).result()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work for lazy collections. You don't want Stream.from(otherStream) to force anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stream uses a lazy builder, so this code doesn’t force the source. I’ve added a test for that.

@julienrf
Copy link
Contributor Author

Now the real question is “do we really want to keep iterator() and newBuilder() in the new collections?”

@julienrf
Copy link
Contributor Author

Closed because of scala/scala#6620

@julienrf julienrf closed this May 14, 2018
martijnhoekstra pushed a commit to martijnhoekstra/scala-collection-compat that referenced this pull request Nov 9, 2022
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