Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added updated to Traverse #4248

Merged
merged 12 commits into from
Jul 1, 2022
Merged

Added updated to Traverse #4248

merged 12 commits into from
Jul 1, 2022

Conversation

nikololiahim
Copy link
Contributor

Implements #4246

@armanbilge armanbilge changed the title Added updated to Traverse Added updated to Traverse Jun 21, 2022
@armanbilge armanbilge added this to the 2.9.0 milestone Jun 21, 2022
johnynek
johnynek previously approved these changes Jun 22, 2022
View reviewed changes
armanbilge
armanbilge reviewed Jun 23, 2022
View reviewed changes
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind rebasing onto #4247? It looks like both PRs have some of the same commits but with different hashes.

@nikololiahim
Copy link
Contributor Author

Not sure what I did here, but seems like nothing was lost @armanbilge

@armanbilge
Copy link
Member

Nice job with that! Indeed, it looks right to me :)

nikololiahim/cats@traverseWithLongIndexM...nikololiahim:traverseUpdated

@armanbilge armanbilge linked an issue Jun 25, 2022 that may be closed by this pull request
Add Traverse#updated #4246
Closed
armanbilge
armanbilge reviewed Jun 27, 2022
View reviewed changes
Copy link
Member

@armanbilge armanbilge left a 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! Also a couple other things I noticed.

tests/shared/src/test/scala/cats/tests/TraverseSuite.scala Outdated Show resolved Hide resolved
core/src/main/scala/cats/instances/vector.scala
@@ -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]] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add a similar override to NonEmptyVector.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added one in f9a00e6.

core/src/main/scala/cats/Traverse.scala
* The behavior is consistent with the Scala collection library's
* `updated` for collections such as `List`.
*/
def updated_[A, B >: A](fa: F[A], idx: Long, b: B): Option[F[B]] = {
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

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 Ref which I assume stands for "reference".

Copy link
Contributor Author

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!

@nikololiahim nikololiahim dismissed a stale review via bdf2d41 June 27, 2022 13:57
armanbilge
armanbilge approved these changes Jun 27, 2022
View reviewed changes
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Great work juggling both PRs!

@nikololiahim
Copy link
Contributor Author

@armanbilge @johnynek Thx for your patience in reviewing/explaining!

@armanbilge armanbilge merged commit 0b7619b into typelevel:main Jul 1, 2022
@armanbilge armanbilge mentioned this pull request Jul 31, 2022
Merged
armanbilge
armanbilge reviewed Aug 17, 2022
View reviewed changes
laws/src/main/scala/cats/laws/TraverseLaws.scala
@@ -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]] = {
Copy link
Member

Choose a reason for hiding this comment

The 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

"traverse identity" -> forAll(laws.traverseIdentity[A, C] _),
"traverse sequential composition" -> forAll(laws.traverseSequentialComposition[A, B, C, X, Y] _),
"traverse parallel composition" -> forAll(laws.traverseParallelComposition[A, B, X, Y] _),
"traverse traverseTap" -> forAll(laws.traverseTap[B, M, X] _),
"traverse derive foldMap" -> forAll(laws.foldMapDerived[A, M] _),
"traverse order consistency" -> forAll(laws.traverseOrderConsistent[A] _),
"traverse ref mapAccumulate" -> forAll(laws.mapAccumulateRef[M, A, C] _),
"traverse ref mapWithIndex" -> forAll(laws.mapWithIndexRef[A, C] _),
"traverse ref traverseWithIndexM" -> forAll(laws.traverseWithIndexMRef[Option, A, C] _),
"traverse ref zipWithIndex" -> forAll(laws.zipWithIndexRef[A, C] _)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I reopen and add them here? Or open a separate PR?

Copy link
Member

Choose a reason for hiding this comment

The 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!!

@nikololiahim nikololiahim mentioned this pull request Aug 17, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnynek johnynek johnynek left review comments

@armanbilge armanbilge armanbilge approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
2.9.0
Development

Successfully merging this pull request may close these issues.

Add Traverse#updated
3 participants
@nikololiahim @armanbilge @johnynek