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 deprecated toTraversable #6841
Conversation
c698647
to
97a756a
Compare
* @return An Iterable containing all elements of this $coll. | ||
*/ | ||
@deprecated("Use toIterable instead", "2.13.0") | ||
def toTraversable: Traversable[A] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this abstract? Could we define @deprecated(..) final def toTraversable: Traversable[A] = toIterable
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I was just mimicking the definition of toIterator
. I gave it a try and removed the abstract definition. It actually makes this change a lot more minimal.
I don't think there's any reason it needs to be abstract. Actually, it looks like overriding toTraversable
was marked deprecated in 3cc99d7. It wasn't possible to actually make it final in 2.12, because it was still being overridden in ParIterableLike
and TraversableProxyLike
, but we can try to do that here now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second @lrytz, just define it once and implement it at the same time.
The toTraversable method wasn't deprecated in 2.12, so it needs to be added back to fix clients who had used it in 2.12. It should just be a proxy for toIterable, because Traversable is now just a type alias for Iterable. After adding toTraversable back, it needs to be marked deprecated in 2.13, so it can be removed in 2.14. This method used to be abstract, but there's no reason it still needs to be going forward. Overriding toTraversable was marked deprecated in 3cc99d7. It should have then been marked final in 2.12, but it probably wasn't possible since toTraversable was still overridden in ParIterableLike and TraversableProxyLike.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good! We could also add it to StringOps
and ArrayOps
, for the sake of completeness.
Is there some |
Oh I didn’t notice that |
Thanks @ashawley! |
I started looking in to whether it would be possible to add I looked at 2.12, and it doesn't look like
import scala.Array
import java.lang.String
// Error: value toTraversable is not a member of Array[T]
Array.empty.toTraversable
// Error: value toTraversable is not a member of String
("": String).toTraversable |
The
toTraversable
method wasn't deprecated in 2.12, so it needs to be added back to fix clients who had used it in 2.12. It should just be a proxy fortoIterable
, becauseTraversable
is now just a type alias forIterable
.After restoring
toTraversable
back, it needs to be marked deprecated in 2.13, so it can be removed in 2.14.