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.

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

Add missing methods to VectorOps #3931

Merged
merged 5 commits into from
Nov 17, 2021

Conversation

satorg
Copy link
Contributor

@satorg satorg commented Jun 25, 2021

Adds methods groupByNev, groupByNevA, scanLeftNev and scanRightNev to VectorOps,
which are similar to the methods found in ListOps.

The implementation is mostly copied from ListOps and adjusted accordingly.

Initial discussion is here.

val nevTraverse = Traverse[NonEmptyVector]

toNev.fold(F.pure(SortedMap.empty[B, NonEmptyVector[A]])) { nev =>
F.map(nevTraverse.traverse(nev)(a => F.tupleLeft(f(a), a))) { vector =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original implementation from ListOps uses the native NonEmptyList#traverse method which does not exist for NonEmptyVector. Therefore Traverse[NonEmptyVector].traverse is used here instead.

Comment on lines +82 to +92
test("scanLeftNev should be consistent with scanLeft")(
forAll { (fa: Vector[Int], b: Int, f: (Int, Int) => Int) =>
assert(fa.scanLeftNev(b)(f).toVector === fa.scanLeft(b)(f))
}
)

test("scanRightNev should be consistent with scanRight")(
forAll { (fa: Vector[Int], b: Int, f: (Int, Int) => Int) =>
assert(fa.scanRightNev(b)(f).toVector === fa.scanRight(b)(f))
}
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two tests do not exist for ListOps's scanLeftNel and scanRightNel methods actually.
I wonder, does it make sense to add similar tests to ListSuite too?
It might be beyond of the PR's scope though...

Copy link
Contributor Author

@satorg satorg Jun 25, 2021

Choose a reason for hiding this comment

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

I took @LukaJCB's reaction as encouragement to do this, so I added the missing tests to ListSuite.

LukaJCB
LukaJCB previously approved these changes Jun 25, 2021
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

djspiewak
djspiewak previously approved these changes Jun 25, 2021
@satorg satorg dismissed stale reviews from djspiewak and LukaJCB via 21c99aa June 25, 2021 21:35
@satorg satorg requested review from djspiewak and LukaJCB June 26, 2021 17:32
@satorg
Copy link
Contributor Author

satorg commented Sep 18, 2021

Hi guys, sorry for the ping, just to confirm – does this PR require anything else to be done?
cc @LukaJCB @djspiewak

Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

Ah, sorry this slipped through the cracks!

@djspiewak
Copy link
Member

Pinging @LukaJCB for a second look

@rossabaker rossabaker merged commit 546f7c0 into typelevel:main Nov 17, 2021
@satorg satorg deleted the extend-vector-syntax branch November 17, 2021 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants