-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Added updated to Traverse
#4248
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
Changes from all commits
b233e77
c1f9ed6
6fe035f
609a4f1
445b949
4b102c6
f3e672f
199d3e8
2cb768d
83e1275
f9a00e6
0942803
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 |
|---|---|---|
|
|
@@ -130,6 +130,13 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances { | |
| final override def traverse[G[_], A, B](fa: Vector[A])(f: A => G[B])(implicit G: Applicative[G]): G[Vector[B]] = | ||
| G.map(Chain.traverseViaChain(fa)(f))(_.toVector) | ||
|
|
||
| final override def updated_[A, B >: A](fa: Vector[A], idx: Long, b: B): Option[Vector[B]] = | ||
|
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. We can add a similar override to
Author
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. Added one in f9a00e6. |
||
| if (idx >= 0L && idx < fa.size.toLong) { | ||
| Some(fa.updated(idx.toInt, b)) | ||
| } else { | ||
| None | ||
| } | ||
|
|
||
| /** | ||
| * This avoids making a very deep stack by building a tree instead | ||
| */ | ||
|
|
@@ -174,6 +181,9 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances { | |
| override def mapWithIndex[A, B](fa: Vector[A])(f: (A, Int) => B): Vector[B] = | ||
| StaticMethods.mapWithIndexFromStrictFunctor(fa, f)(this) | ||
|
|
||
| override def mapWithLongIndex[A, B](fa: Vector[A])(f: (A, Long) => B): Vector[B] = | ||
| StaticMethods.mapWithLongIndexFromStrictFunctor(fa, f)(this) | ||
|
|
||
| override def zipWithIndex[A](fa: Vector[A]): Vector[(A, Int)] = | ||
| fa.zipWithIndex | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -129,6 +129,43 @@ trait TraverseLaws[F[_]] extends FunctorLaws[F] with FoldableLaws[F] with Unorde | |||||||||||||||||||||
| val rhs = F.map(F.mapWithIndex(fa)((a, i) => (a, i)))(f) | ||||||||||||||||||||||
| lhs <-> rhs | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def mapWithLongIndexRef[A, B](fa: F[A], f: (A, Long) => B): IsEq[F[B]] = { | ||||||||||||||||||||||
|
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. @nikololiahim I just realized, we added the laws but not the tests 😅 I completely forgot about those, so sorry! cats/laws/src/main/scala/cats/laws/discipline/TraverseTests.scala Lines 74 to 83 in 0942803
Author
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. Should I reopen and add them here? Or open a separate PR?
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. Whatever works, although I'm not sure if it's possible to re-open a merged PR :) thanks!! |
||||||||||||||||||||||
| val lhs = F.mapWithLongIndex(fa)(f) | ||||||||||||||||||||||
| val rhs = F.traverse(fa)(a => State((s: Long) => (s + 1, f(a, s)))).runA(0L).value | ||||||||||||||||||||||
| lhs <-> rhs | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def traverseWithLongIndexMRef[G[_], A, B](fa: F[A], f: (A, Long) => G[B])(implicit G: Monad[G]): IsEq[G[F[B]]] = { | ||||||||||||||||||||||
| val lhs = F.traverseWithLongIndexM(fa)(f) | ||||||||||||||||||||||
| val rhs = F.traverse(fa)(a => StateT((s: Long) => G.map(f(a, s))(b => (s + 1, b)))).runA(0L) | ||||||||||||||||||||||
| lhs <-> rhs | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def zipWithLongIndexRef[A, B](fa: F[A], f: ((A, Long)) => B): IsEq[F[B]] = { | ||||||||||||||||||||||
| val lhs = F.map(F.zipWithLongIndex(fa))(f) | ||||||||||||||||||||||
| val rhs = F.map(F.mapWithLongIndex(fa)((a, i) => (a, i)))(f) | ||||||||||||||||||||||
| lhs <-> rhs | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def updatedRef[A, B >: A](fa: F[A], idx: Long, b: B): IsEq[Option[F[B]]] = { | ||||||||||||||||||||||
| val lhs = F.updated_(fa, idx, b) | ||||||||||||||||||||||
| val rhs = | ||||||||||||||||||||||
| if (idx < 0L) | ||||||||||||||||||||||
| None | ||||||||||||||||||||||
| else | ||||||||||||||||||||||
| F.mapAccumulate(0L, fa)((i, a) => | ||||||||||||||||||||||
| if (i == idx) | ||||||||||||||||||||||
| (i + 1, b) | ||||||||||||||||||||||
| else | ||||||||||||||||||||||
| (i + 1, a) | ||||||||||||||||||||||
| ) match { | ||||||||||||||||||||||
| case (i, fb) if i > idx => Some(fb) | ||||||||||||||||||||||
| case _ => None | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| lhs <-> rhs | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| object TraverseLaws { | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
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.
Sorry, I didn't realize we are missing a law for this! It can be like the other laws, just verifying against the reference implementation.
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.
@armanbilge I am a little lost here, what should the reference implementation be in this case?
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.
Ended up adding one of the previous implementations in bdf2d41. Is it okay?
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.
Ah, the reference implementation should be the same as the default implementation :) basically, it's a way to make sure that if someone overrides it (like we do in
Vector) it matches what the default implementation would do.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.
Btw, sorry I wrote that so confusingly, I should have just said "default implementation". I was thinking about how these laws end in
Refwhich I assume stands for "reference".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.
Ah, so that's why they are all the same as default. I thought it was weird that they are all just the same as the trait impl. Like, what are we even testing here? The fact that some instance may override certain default impl totally slipped from my head. Now it all makes sense, thank you!