Implicit builders on top of #110 #112
Conversation
|
|
||
| object BuildFrom extends BuildFromLowPriority { | ||
|
|
||
| type Aux[From, A, To0] = BuildFrom[From, A] { type To = To0 } |
There was a problem hiding this comment.
I wouldn't collide with Shapeless' name here. The type signature is consistent with something called some variant on BuildSpecification. That's obviously too long, but Aux is too uninformative (in addition to the slight chance of confusion from shadowing).
There was a problem hiding this comment.
This type makes things really confusing. Can we get rid of it and use either type members or type parameters everywhere?
There was a problem hiding this comment.
I disagree. According to the context it can be more convenient to use type members or type parameters. That’s why this pattern is so prevalent in shapeless.
I can find a better name than Aux, though.
There was a problem hiding this comment.
I agree. I would try to use parameters everywhere because that's how we started. Also, if we bring back CanBuildFrom in a way, it's best not to do arbitrary changes to it.
There was a problem hiding this comment.
The whole thing looks still very complex to me. Let's see whether we can simplify by going to all parameters.
There was a problem hiding this comment.
I think Build[From, A, To] ends up seeming too complex even though it is "all parameters". If the Aux-style transformation of a member into a parameter is something that can be behind the scenes in almost every case, I think that would give a better user experience. If people get to interact with BuildFrom[From, A] instead of a triply parameterized thing, I think they'll be happier. Also, the triply parameterized one is kind of misleading because we don't fill in most of Build[From, A, To] in an interesting way for specific choices of From and To. Generally if we know what we want to go To we don't care where we came from; and if we know where we come From there is only a single choice of where to go To.
| import scala.math.Ordering | ||
| import java.lang.String | ||
|
|
||
| class BuildFromTest { |
There was a problem hiding this comment.
I really like the usage here. There's a bit of confusing magic about "how does it even know the collection type I mean when I didn't really say what it was?"
But the magic works and it feels way more usable to me than CBF did. Especially since xs is now just a plain Iterable[Option[A]] instead of some generified parameterized thingy that might be hard to get right.
There was a problem hiding this comment.
Note that this style is not required, these examples would work as well as follows:
// def optionSequence[A](xs: Iterable[Option[A]])(implicit bf: BuildFrom[xs.type, A]): Option[bf.To]
def optionSequence[A, CC[X] <: Iterable[X]](xs: CC[Option[A]])(implicit bf: BuildFrom[CC[Option[A]], A]): Option[bf.To]Also, users can choose to use a path dependent type (bf.To) or to take an additional To type parameter (by using BuildFrom.Aux -- which I could rename to something else).
There was a problem hiding this comment.
And you should be able to do the same with the current CanBuildFrom. It certainly works with the new BuildFrom. I have examples of both styles in my PR.
|
I haven't looked at Stefan's latest version yet, but I'm quite content with the usage of this version. It's got one bit of surprising magic (that the type lookups are driven off of |
| @@ -27,7 +27,7 @@ class ArrayOps[A](val xs: Array[A]) | |||
| protected[this] def fromTaggedIterable[B: ClassTag](coll: Iterable[B]): Array[B] = coll.toArray[B] | |||
| protected[this] def fromSpecificIterable(coll: Iterable[A]): Array[A] = coll.toArray[A](elemTag) | |||
|
|
|||
| protected[this] def newSpecificBuilder() = new GrowableBuilder(ArrayBuffer.empty[A]).mapResult(_.toArray(elemTag)) | |||
| protected[this] def newSpecificBuilder() = ArrayBuffer.newBuilder[A]().mapResult(_.toArray(elemTag)) | |||
There was a problem hiding this comment.
This could avoid copying if the size matches. We may also consider passing a size hint to newSpecificBuilder (but it's probably not as useful in the new design where most operations use fromSpecificIterable instead).
|
Overall this has the advantage that it is relatively short. But I find it confusing that we have so many different types concerned with Builders. |
I rebased #91 on top of #110.
The goal of this PR is to be easily comparable to #97, which solves the same problems (see their tests) in a different way.
The first commit shows the essence of the approach: it introduces a
CanBuildtrait and add implicit instances of it in all strict factories. The remaining of the commit makes mutable collection factories strict again and then adds several tests.The second commit moves the
BuildFromandBuildToabstractions from the tests to the main project (they could live in a different project, if needed…). The key idea is to show thatCanBuild(introduced in the first commit) is enough to derive useful other abstractions such asBuildFromandBuildTo(these ones could live in their own project, they have no impact on the core).