-
Notifications
You must be signed in to change notification settings - Fork 72
Conversation
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.
Thanks for putting this work together. I like the overall idea. I left some minor comments.
@@ -244,6 +244,8 @@ trait IterableOps[+A, +CC[X], +C] extends Any { | |||
/** A view representing the elements of this collection. */ | |||
def view: View[A] = View.fromIterator(coll.iterator()) | |||
|
|||
def transform[B](vt: Iterable[A] => View[B]): CC[B] = fromIterable(vt(coll)) |
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’m not a fan of the transform
name. Maybe transformView
?
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.
Except in this case it starts out as an Iterable
and ends up as one. The intermediate representation as View
s is really an implementation detail. The closest equivalent in Clojure's transducer kit would be sequence
, but that's even worse.
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.
Actually, whether it ends up as a View
, depends only on whether it started out as one, which is nice.
def drop(n: Int): VT[A,B] = v => new View.Drop(vt(v),n) | ||
def collect[C](pf: scala.PartialFunction[B,C]): VT[A,C] = v => new View.Collect(vt(v),pf) | ||
def zipwithIndex: VT[A,(B,Int)] = v => new View.ZipWithIndex[B](vt(v)) | ||
} |
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 choose to use a type alias + an implicit class rather than a proper class?
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 did have it as a real class originally, and after playing around a bit, the typeclass approach seemed neater. I'm not committed to this though. I'll fiddle around some more.
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.
It ends up looking like
abstract class ViewTransformer[-A,+B] {
def apply(v: Iterable[A]): View[B]
def map[C](f: B => C) = new ViewTransformer[A,C] {
def apply(v: Iterable[A]): View[C] = new View.Map(ViewTransformer.this(v),f)
}
...
}
which is fine I guess, but feels clunkier (to me).
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 think this form is more easily grasped by a larger fraction of Scala users (as it depends on a smaller more widely used set of language features). Elegance is fine, but approachability is usually more important.
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.
What’s clunkier exactly? You can also define a factory method in the ViewTransformer
companion:
object ViewTransformer {
def apply[A, B](f: Iterable[A] => View[B]): ViewTransformer[A, B] = …
}
class ViewTransformer[-A, +B] {
def apply(as: Iterable[A]): View[B]
def map[C](f: B => C) = ViewTransformer(it => View.Map(apply(v)), f)
}
Or, you can also use the “lambda” syntax (which has been generalized to SAM types):
trait ViewTransformer[-A, +B] {
def apply(as: Iterable[A]): View[B]
def map[C](f: B => C): ViewTransformer[A, C] = as => View.Map(apply(as), f)
}
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 didn't realize we could use lambda sugar for any SAM. I'll do that...
def collect[C](pf: scala.PartialFunction[B,C]): VT[A,C] = v => new View.Collect(vt(v),pf) | ||
def zipwithIndex: VT[A,(B,Int)] = v => new View.ZipWithIndex[B](vt(v)) | ||
} | ||
def source[A]: VT[A,A] = (v: Iterable[A]) => View.fromIterable(v) |
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’m not a big fan of the source
name. What about id
(since this view transformer preserves the same collection)?
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.
Hah! It was originally called id
. You seem to have inferred all my first draft ideas. Anyway, I'm fine changing it back.
I’m wondering if we really need If we go back to a form where val x: ViewTransformer[Int, Double] = xs => xs.map(i => i * 2).flatMap(i => Seq(i + 0.0, i + 0.5))
val x2: ViewTransformer[Double, (String, Int)] = xs => xs.collect { case d if d < 3.0 => s"Hey $d" }.zipWithIndex
xs.transform(x andThen x2) instead of the current: val x = id[Int].map{ i => i * 2 }.flatMap { i => collection.immutable.Seq(i + 0.0, i + 0.5) }
val x2 = id[scala.Double].collect { case d if d < 3.0 => s"Hey $d"}.zipwithIndex
xs.transform(x andThen x2) |
closing all open pull requests in this repo. perhaps this could move forward in scala/scala, where the code now lives |
POC for composing view transformations, ala transducers.