Restore Array.deep from ArrayLike #6912
Conversation
julienrf
changed the title
|
|
||
| /** Creates a possible nested `IndexedSeq` which consists of all the elements | ||
| * of this array. If the elements are arrays themselves, the `deep` transformation | ||
| * is applied recursively to them. The `stringPrefix` of the `IndexedSeq` is |
There was a problem hiding this comment.
Should be className instead of stringPrefix
| case x: Array[Boolean] => deep((new mutable.ArraySeq.ofBoolean(x) ).asInstanceOf[mutable.ArraySeq[Boolean]]) | ||
| case x: Array[Unit] => deep((new mutable.ArraySeq.ofUnit(x) ).asInstanceOf[mutable.ArraySeq[Unit]] ) | ||
| case x => x | ||
| } |
There was a problem hiding this comment.
The current implementation in 2.12.x seems simpler: https://github.com/scala/scala/blob/2.12.x/src/library/scala/collection/mutable/ArrayLike.scala#L41-L44
Is there a reason to have all these cases?
|
Can you also fix the commit message? |
MasseGuillaume
force-pushed
the
bug/10985
branch
2 times, most recently
from
c92030f
to
f5490b4
Compare
MasseGuillaume
force-pushed
the
bug/10985
branch
from
f5490b4
to
8ae7713
Compare
|
@julienrf done, ci is ✔️ |
| implicit class ArrayDeep(val a: Array[_]) extends AnyVal { | ||
| def deep: collection.IndexedSeq[Any] = prettyArray(a) | ||
| def prettyArray: collection.IndexedSeq[Any] = prettyArray0(a) |
There was a problem hiding this comment.
Actually, I’m thinking about something: the less code we have in this file, the more compatible with Scala.js partests are. Currently, all the tests that use the deep method that you renamed to prettyArray are blacklisted in Scala.js because this Util.scala file is not used by Scala.js, therefore the partests that use features defined in that file don’t compile.
Maybe it’s an opportunity to remove this prettyArray thing at all. Do we really need a method that wraps arbitrarily nested levels of Arrays to IndexedSeqs? I guess we usually have one or two levels of nested arrays, so the fix consists in manually calling .toSeq or .map(_.toSeq).toSeq.
There was a problem hiding this comment.
What if we keep Array.deep without deprecation. This method is useful to print an array or check for equality.
There was a problem hiding this comment.
I’m not sure. To print an array I find it simple to call .toSeq. To check for equality we can use Java’s Arrays.deepEquals. What do other people think?
There was a problem hiding this comment.
Array(Array(1)).toSeq.toString // WrappedArray([I@6fc1a8f6)
There was a problem hiding this comment.
If we keep it, deep is certainly a not a good name. Are there any other use cases than toString? If not, there should instead be a method that returns the string representation directly.
Personally, I think the method is not widely useful enough for the standard libarary.
There was a problem hiding this comment.
It's for pretty-printing and checking for equality in tests. If you use Arrays.deepEquals, you only have one.
There was a problem hiding this comment.
The very fact that deep() returns an IndexedSeq[Any] is a huge smell. It's basically not type parametric. If I manipulate an Array[T] and I call deep() on it, I am at the mercy of some of the Ts being Arrays. It's the same huge problem that JavaScript Promises face.
Kill it.
| @@ -108,7 +108,7 @@ object Test4 { | |||
| import java.lang.reflect.AnnotatedElement | |||
| def printSourceAnnotation(a: Annotation): Unit = { | |||
| val ann = a.asInstanceOf[SourceAnnotation] | |||
| println("@test.SourceAnnotation(mails=" + ann.mails.deep.mkString("{", ",", "}") + | |||
| println("@test.SourceAnnotation(mails=" + ann.mails.prettyArray.mkString("{", ",", "}") + | |||
There was a problem hiding this comment.
For instance, here we can just remove deep: ann.mails.mkString("{", ",", "}")
| @@ -71,7 +71,7 @@ object Test2 { | |||
| println() | |||
|
|
|||
| def loadArray[T](x: Array[Byte])(implicit t: reflect.ClassTag[Array[T]]) = | |||
| load[Array[T]](x)(t).deep.toString | |||
| load[Array[T]](x)(t).prettyArray.toString | |||
There was a problem hiding this comment.
Here we can replace deep with toSeq.
| @@ -71,7 +71,7 @@ object Test2 { | |||
| println() | |||
|
|
|||
| def loadArray[T](x: Array[Byte])(implicit m: reflect.Manifest[Array[T]]) = | |||
| load[Array[T]](x)(m).deep.toString | |||
| load[Array[T]](x)(m).prettyArray.toString | |||
|
Why did you close the PR? |
|
@julienrf It looks like no one is on board with the idea of keeping this method. We should add this in |
|
I don’t think we should add it to scala-collection-compat (see my comment there). But we can simply not use it and manually migrate the projects that used to rely on it (like our partests) |
fix scala/bug#10985