-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Adding NonEmptyVector #1137
Adding NonEmptyVector #1137
Changes from 3 commits
c6c8ee0
fb7988f
b715ee7
74e8a40
33b845f
d237bdb
a79bc57
a9ceb60
e35ada2
696e269
e173928
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,178 @@ | ||
package cats | ||
package data | ||
|
||
import scala.annotation.tailrec | ||
import scala.collection.immutable.VectorBuilder | ||
import cats.std.vector._ | ||
|
||
/** | ||
* A data type which represents a single element (head) and a Vector | ||
* (tail). This can be used to represent a Vector which is guaranteed | ||
* to not be empty. | ||
*/ | ||
final case class NonEmptyVector[A] private (vector: Vector[A]) { | ||
|
||
def unwrap: Vector[A] = vector | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since people can just call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could call the member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to |
||
|
||
def head: A = vector.head | ||
|
||
def tail: Vector[A] = vector.tail | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like it's worth supporting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or both? :-) But agree on wanting indexed access. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @non you mean I'd be in favor of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! Sorry, in my defense it was late! ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not have: def get(i: Int): Option[A]
// This is unsafe, it may throw on out-of-bounds, see `get` for a safe variant.
def apply(i: Int): A for updated I would like: def updatedOption(i: Int, a: A): Option[NonEmptyVector[A]]
// this will throw is i is out of bounds.
def updated(i: Int, a: A): NonEmptyVector[A] I feel like adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. alternatively, since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is tough. I think that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see your point on safety, but there is also a cost (especially to reviewers of future code) of using names that are unsafe on Vector to be safe here (if they change the type signature). My bias is using Maybe I would say: when we can safely follow standard naming conventions, we should. Otherwise, if we want to add methods that can throw, use
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @johnynek everything that you said sounds good to me. So it sounds like it'll be |
||
/** | ||
* remove elements not matching the predicate | ||
*/ | ||
def filter(f: A => Boolean): Vector[A] = vector.filter(f) | ||
|
||
/** | ||
* Append another NonEmptyVector to this | ||
*/ | ||
def combine(other: NonEmptyVector[A]): NonEmptyVector[A] = NonEmptyVector(vector ++ other.vector) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably also want a variant of this that works with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And even though this will be called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for |
||
|
||
/** | ||
* find the first element matching the predicate, if one exists | ||
*/ | ||
def find(f: A => Boolean): Option[A] = vector.find(f) | ||
|
||
/** | ||
* Check whether at least one element satisfies the predicate. | ||
*/ | ||
def exists(f: A => Boolean): Boolean = vector.exists(f) | ||
|
||
/** | ||
* Check whether all elements satisfy the predicate. | ||
*/ | ||
def forall(f: A => Boolean): Boolean = vector.forall(f) | ||
|
||
/** | ||
* Left-associative fold using f. | ||
*/ | ||
def foldLeft[B](b: B)(f: (B, A) => B): B = | ||
vector.foldLeft(b)(f) | ||
|
||
/** | ||
* Right-associative fold using f. | ||
*/ | ||
def foldRight[B](lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] = | ||
Eval.defer(vector.foldRight(lb)(f)) | ||
|
||
/** | ||
* Applies f to all the elements | ||
*/ | ||
def map[B](f: A => B): NonEmptyVector[B] = | ||
NonEmptyVector(vector.map(f)) | ||
|
||
/** | ||
* Applies f to all elements and combines the result | ||
*/ | ||
def flatMap[B](f: A => NonEmptyVector[B]): NonEmptyVector[B] = | ||
NonEmptyVector(vector.flatMap(a => f(a).vector)) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think they should be called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could add |
||
/** | ||
* Typesafe equality operator. | ||
* | ||
* This method is similar to == except that it only allows two | ||
* NonEmptyVector[A] values to be compared to each other, and uses | ||
* equality provided by Eq[_] instances, rather than using the | ||
* universal equality provided by .equals. | ||
*/ | ||
def ===(that: NonEmptyVector[A])(implicit A: Eq[A]): Boolean = Eq[Vector[A]].eqv(vector, that.vector) | ||
|
||
/** | ||
* Typesafe stringification method. | ||
* | ||
* This method is similar to .toString except that it stringifies | ||
* values according to Show[_] instances, rather than using the | ||
* universal .toString method. | ||
*/ | ||
def show(implicit A: Show[A]): String = | ||
s"NonEmptyVector(${A.show(head)}, ${Show[Vector[A]].show(tail)})" | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this show correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better than the |
||
|
||
private[data] sealed trait NonEmptyVectorInstances extends NonEmptyVectorLowPriority2 { | ||
|
||
implicit def catsDataEqForNonEmptyVector[A](implicit A: Eq[A]): Eq[NonEmptyVector[A]] = | ||
new Eq[NonEmptyVector[A]]{ | ||
def eqv(x: NonEmptyVector[A], y: NonEmptyVector[A]): Boolean = x === y | ||
} | ||
|
||
implicit def catsDataShowForNonEmptyVector[A](implicit A: Show[A]): Show[NonEmptyVector[A]] = | ||
Show.show[NonEmptyVector[A]](_.show) | ||
|
||
implicit def catsDataSemigroupKForNonEmptyVector: SemigroupK[NonEmptyVector] = | ||
new SemigroupK[NonEmptyVector] { | ||
def combineK[A](a: NonEmptyVector[A], b: NonEmptyVector[A]): NonEmptyVector[A] = | ||
a combine b | ||
} | ||
|
||
implicit def catsDataSemigroupForNonEmptyVector[A]: Semigroup[NonEmptyVector[A]] = | ||
catsDataSemigroupKForNonEmptyVector.algebra | ||
|
||
implicit def catsDataReducibleForNonEmptyVector: Reducible[NonEmptyVector] = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should be able to combine a bunch of these instances into 1 like we do with the instances for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also when you do this you should be able to change it from a |
||
new NonEmptyReducible[NonEmptyVector, Vector] { | ||
override def split[A](fa: NonEmptyVector[A]): (A, Vector[A]) = (fa.head, fa.tail) | ||
|
||
override def size[A](fa: NonEmptyVector[A]): Long = 1 + fa.tail.size.toLong | ||
} | ||
|
||
implicit def catsDataMonadForNonEmptyVector: Monad[NonEmptyVector] = | ||
new Monad[NonEmptyVector] { | ||
override def map[A, B](fa: NonEmptyVector[A])(f: A => B): NonEmptyVector[B] = | ||
fa map f | ||
|
||
def pure[A](x: A): NonEmptyVector[A] = | ||
NonEmptyVector(x, Vector.empty) | ||
|
||
def flatMap[A, B](fa: NonEmptyVector[A])(f: A => NonEmptyVector[B]): NonEmptyVector[B] = | ||
fa flatMap f | ||
} | ||
} | ||
|
||
trait NonEmptyVectorLowPriority0 { | ||
|
||
implicit def catsDataComonadForNonEmptyVector: Comonad[NonEmptyVector] = new Comonad[NonEmptyVector] { | ||
|
||
def coflatMap[A, B](fa: NonEmptyVector[A])(f: NonEmptyVector[A] => B): NonEmptyVector[B] = { | ||
@tailrec def consume(as: Vector[A], buf: VectorBuilder[B]): Vector[B] = | ||
as match { | ||
case a +: as => consume(as, buf += f(NonEmptyVector(a, as))) | ||
case _ => buf.result() | ||
} | ||
NonEmptyVector(f(fa), consume(fa.tail, new VectorBuilder[B])) | ||
} | ||
|
||
|
||
def extract[A](fa: NonEmptyVector[A]): A = fa.head | ||
|
||
def map[A, B](fa: NonEmptyVector[A])(f: A => B): NonEmptyVector[B] = fa map f | ||
} | ||
} | ||
|
||
trait NonEmptyVectorLowPriority1 extends NonEmptyVectorLowPriority0 { | ||
implicit def catsDataFunctorForNonEmptyVector: Functor[NonEmptyVector] = | ||
new Functor[NonEmptyVector] { | ||
def map[A, B](fa: NonEmptyVector[A])(f: A => B): NonEmptyVector[B] = | ||
fa map f | ||
} | ||
|
||
} | ||
|
||
trait NonEmptyVectorLowPriority2 extends NonEmptyVectorLowPriority1 { | ||
implicit def catsDataTraverseForNonEmptyVector: Traverse[NonEmptyVector] = | ||
new Traverse[NonEmptyVector] { | ||
def traverse[G[_], A, B](fa: NonEmptyVector[A])(f: (A) => G[B])(implicit G: Applicative[G]): G[NonEmptyVector[B]] = | ||
G.map2Eval(f(fa.head), Always(Traverse[Vector].traverse(fa.tail)(f)))(NonEmptyVector(_, _)).value | ||
|
||
|
||
def foldLeft[A, B](fa: NonEmptyVector[A], b: B)(f: (B, A) => B): B = | ||
fa.foldLeft(b)(f) | ||
|
||
def foldRight[A, B](fa: NonEmptyVector[A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] = | ||
fa.foldRight(lb)(f) | ||
} | ||
} | ||
|
||
object NonEmptyVector extends NonEmptyVectorInstances { | ||
|
||
def apply[A](head: A, tail: Vector[A]): NonEmptyVector[A] = NonEmptyVector(head +: tail) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to support this as: def apply[A](body: Vector[A], last: A): NonEmptyVector[A] = NonEmptyVector(body :+ last) In general I don't like treating non-empty vector as a head/tail pair because it's so inefficient -- prepending to a vector is O(n) and reallocates the entire vector. Appending is a lot more efficient, so in this case I think it makes more sense. Honestly I'd probably prefer an interface like: def apply[A](xs: Vector[A]): Option[NonEmptyVector[A]] = ... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I probably missed something. I have been holding the belief that according to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might propose the following instantiation methods: // to be consistent with var-arg `apply` methods for other collections
def apply[A](head: A, tail: A*): NonEmptyVector[A]
def fromVector[A](vector: Vector[A]): Option[NonEmptyVector[A]]
// this will throw an exception if the vector is empty
def fromVectorUnsafe[A](vector: Vector[A]): NonEmptyVector[A]
def prepend[A](head: A, tail: Vector[A]): NonEmptyVector[A]
def append[A](vector: Vector[A], last: A): NonEmptyVector[A] I think that would cover all of the use-cases. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right @kailuowang. I forgot that vectors maintain a start index to allow this kind of thing to be effectively constant time. I was wrong. I think maybe I got confused because for awhile |
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,10 +21,4 @@ type NonEmptyList[A] = OneAnd[List, A] | |
|
||
which is the actual implementation of non-empty lists in cats. By | ||
having the higher kinded type parameter `F[_]`, `OneAnd` is also able | ||
to represent other "non-empty" data structures, e.g. | ||
|
||
```tut:silent | ||
import cats.data.OneAnd | ||
|
||
type NonEmptyVector[A] = OneAnd[Vector, A] | ||
``` | ||
to represent other "non-empty" data structures. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't feel strongly about this but shall we replace this example with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that after this PR (or possibly as a part of this PR) we can probably rip out |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,171 @@ | ||
package cats | ||
package tests | ||
|
||
import cats.kernel.laws.{GroupLaws, OrderLaws} | ||
|
||
import cats.data.NonEmptyVector | ||
import cats.laws.discipline.{ComonadTests, FunctorTests, SemigroupKTests, FoldableTests, MonadTests, SerializableTests, CartesianTests, TraverseTests, ReducibleTests} | ||
import cats.laws.discipline.arbitrary._ | ||
|
||
class NonEmptyVectorTests extends CatsSuite { | ||
// Lots of collections here.. telling ScalaCheck to calm down a bit | ||
implicit override val generatorDrivenConfig: PropertyCheckConfiguration = | ||
PropertyCheckConfig(maxSize = 5, minSuccessful = 20) | ||
|
||
checkAll("NonEmptyVector[Int]", OrderLaws[NonEmptyVector[Int]].eqv) | ||
|
||
checkAll("NonEmptyVector[Int] with Option", TraverseTests[NonEmptyVector].traverse[Int, Int, Int, Int, Option, Option]) | ||
checkAll("Traverse[NonEmptyVector[A]]", SerializableTests.serializable(Traverse[NonEmptyVector])) | ||
|
||
checkAll("NonEmptyVector[Int]", ReducibleTests[NonEmptyVector].reducible[Option, Int, Int]) | ||
checkAll("Reducible[NonEmptyVector]", SerializableTests.serializable(Reducible[NonEmptyVector])) | ||
|
||
implicit val iso = CartesianTests.Isomorphisms.invariant[NonEmptyVector](NonEmptyVector.catsDataFunctorForNonEmptyVector) | ||
|
||
// Test instances that have more general constraints | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to add braces for separate scopes for the following tests. That's just done for tests where there's an abstract |
||
checkAll("NonEmptyVector[Int]", CartesianTests[NonEmptyVector].cartesian[Int, Int, Int]) | ||
checkAll("Cartesian[NonEmptyVector]", SerializableTests.serializable(Cartesian[NonEmptyVector])) | ||
} | ||
|
||
{ | ||
checkAll("NonEmptyVector[Int]", FunctorTests[NonEmptyVector].functor[Int, Int, Int]) | ||
checkAll("Functor[NonEmptyVector]", SerializableTests.serializable(Functor[NonEmptyVector])) | ||
} | ||
|
||
{ | ||
checkAll("NonEmptyVector[Int]", SemigroupKTests[NonEmptyVector].semigroupK[Int]) | ||
checkAll("NonEmptyVector[Int]", GroupLaws[NonEmptyVector[Int]].semigroup) | ||
checkAll("SemigroupK[NonEmptyVector]", SerializableTests.serializable(SemigroupK[NonEmptyVector])) | ||
checkAll("Semigroup[NonEmptyVector[Int]]", SerializableTests.serializable(Semigroup[NonEmptyVector[Int]])) | ||
} | ||
|
||
{ | ||
checkAll("NonEmptyVector[Int]", FoldableTests[NonEmptyVector].foldable[Int, Int]) | ||
checkAll("Foldable[NonEmptyVector]", SerializableTests.serializable(Foldable[NonEmptyVector])) | ||
} | ||
|
||
{ | ||
// Test functor and subclasses don't have implicit conflicts | ||
implicitly[Functor[NonEmptyVector]] | ||
implicitly[Monad[NonEmptyVector]] | ||
implicitly[Comonad[NonEmptyVector]] | ||
} | ||
|
||
{ | ||
checkAll("NonEmptyVector[Int]", MonadTests[NonEmptyVector].monad[Int, Int, Int]) | ||
checkAll("Monad[NonEmptyVector]", SerializableTests.serializable(Monad[NonEmptyVector])) | ||
} | ||
|
||
{ | ||
checkAll("NonEmptyVector[Int]", ComonadTests[NonEmptyVector].comonad[Int, Int, Int]) | ||
checkAll("Comonad[NonEmptyVector]", SerializableTests.serializable(Comonad[NonEmptyVector])) | ||
} | ||
|
||
|
||
test("size is consistent with toList.size") { | ||
forAll { (nonEmptyVector: NonEmptyVector[Int]) => | ||
nonEmptyVector.size should === (nonEmptyVector.toList.size.toLong) | ||
} | ||
} | ||
|
||
|
||
test("Show is not empty and is formatted as expected") { | ||
forAll { (nonEmptyVector: NonEmptyVector[Int]) => | ||
nonEmptyVector.show.nonEmpty should === (true) | ||
nonEmptyVector.show.startsWith("NonEmptyVector(") should === (true) | ||
nonEmptyVector.show should === (implicitly[Show[NonEmptyVector[Int]]].show(nonEmptyVector)) | ||
nonEmptyVector.show.contains(nonEmptyVector.head.show) should === (true) | ||
} | ||
} | ||
|
||
test("Show is formatted correctly") { | ||
val nonEmptyVector = NonEmptyVector("Test", Vector.empty) | ||
nonEmptyVector.show should === ("NonEmptyVector(Test, Vector())") | ||
} | ||
|
||
test("Creating NonEmptyVector + unwrap is identity") { | ||
forAll { (i: Int, tail: Vector[Int]) => | ||
val vector = i +: tail | ||
val nonEmptyVector = NonEmptyVector(i, tail) | ||
vector should === (nonEmptyVector.unwrap) | ||
} | ||
} | ||
|
||
test("NonEmptyVector#filter is consistent with NonEmptyVector#filter") { | ||
forAll { (nonEmptyVector: NonEmptyVector[Int], p: Int => Boolean) => | ||
val vector = nonEmptyVector.unwrap | ||
nonEmptyVector.filter(p) should === (vector.filter(p)) | ||
} | ||
} | ||
|
||
test("NonEmptyVector#find is consistent with NonEmptyVector#find") { | ||
forAll { (nonEmptyVector: NonEmptyVector[Int], p: Int => Boolean) => | ||
val vector = nonEmptyVector.unwrap | ||
nonEmptyVector.find(p) should === (vector.find(p)) | ||
} | ||
} | ||
|
||
test("NonEmptyVector#exists is consistent with NonEmptyVector#exists") { | ||
forAll { (nonEmptyVector: NonEmptyVector[Int], p: Int => Boolean) => | ||
val vector = nonEmptyVector.unwrap | ||
nonEmptyVector.exists(p) should === (vector.exists(p)) | ||
} | ||
} | ||
|
||
test("NonEmptyVector#forall is consistent with NonEmptyVector#forall") { | ||
forAll { (nonEmptyVector: NonEmptyVector[Int], p: Int => Boolean) => | ||
val vector = nonEmptyVector.unwrap | ||
nonEmptyVector.forall(p) should === (vector.forall(p)) | ||
} | ||
} | ||
|
||
test("NonEmptyVector#map is consistent with NonEmptyVector#map") { | ||
forAll { (nonEmptyVector: NonEmptyVector[Int], p: Int => String) => | ||
val vector = nonEmptyVector.unwrap | ||
nonEmptyVector.map(p).unwrap should === (vector.map(p)) | ||
} | ||
} | ||
|
||
test("reduceLeft consistent with foldLeft") { | ||
forAll { (nonEmptyVector: NonEmptyVector[Int], f: (Int, Int) => Int) => | ||
nonEmptyVector.reduceLeft(f) should === (nonEmptyVector.tail.foldLeft(nonEmptyVector.head)(f)) | ||
} | ||
} | ||
|
||
test("reduceRight consistent with foldRight") { | ||
forAll { (nonEmptyVector: NonEmptyVector[Int], f: (Int, Eval[Int]) => Eval[Int]) => | ||
nonEmptyVector.reduceRight(f).value should === (nonEmptyVector.tail.foldRight(nonEmptyVector.head)((a, b) => f(a, Now(b)).value)) | ||
} | ||
} | ||
|
||
test("reduce consistent with fold") { | ||
forAll { (nonEmptyVector: NonEmptyVector[Int]) => | ||
nonEmptyVector.reduce should === (nonEmptyVector.fold) | ||
} | ||
} | ||
|
||
test("reduce consistent with reduceK") { | ||
forAll { (nonEmptyVector: NonEmptyVector[Option[Int]]) => | ||
nonEmptyVector.reduce(SemigroupK[Option].algebra[Int]) should === (nonEmptyVector.reduceK) | ||
} | ||
} | ||
|
||
test("reduceLeftToOption consistent with foldLeft + Option") { | ||
forAll { (nonEmptyVector: NonEmptyVector[Int], f: Int => String, g: (String, Int) => String) => | ||
val expected = nonEmptyVector.tail.foldLeft(Option(f(nonEmptyVector.head))) { (opt, i) => | ||
opt.map(s => g(s, i)) | ||
} | ||
nonEmptyVector.reduceLeftToOption(f)(g) should === (expected) | ||
} | ||
} | ||
|
||
test("reduceRightToOption consistent with foldRight + Option") { | ||
forAll { (nonEmptyVector: NonEmptyVector[Int], f: Int => String, g: (Int, Eval[String]) => Eval[String]) => | ||
val expected = nonEmptyVector.tail.foldRight(Option(f(nonEmptyVector.head))) { (i, opt) => | ||
opt.map(s => g(i, Now(s)).value) | ||
} | ||
nonEmptyVector.reduceRightToOption(f)(g).value should === (expected) | ||
} | ||
} | ||
} |
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.
This description should be changed, since we're just wrapping a vector.