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
CU-fh2vcf - Deprecate mapN and fix zip for collections #2324
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.
LGTM thanks @danimontoya ❤️
arrow-libs/core/arrow-core-data/src/main/kotlin/arrow/core/NonEmptyList.kt
Outdated
Show resolved
Hide resolved
arrow-libs/core/arrow-core-data/src/main/kotlin/arrow/core/NonEmptyList.kt
Outdated
Show resolved
Hide resolved
arrow-libs/core/arrow-core-data/src/main/kotlin/arrow/core/NonEmptyList.kt
Outdated
Show resolved
Hide resolved
arrow-libs/core/arrow-core-data/src/main/kotlin/arrow/core/NonEmptyList.kt
Outdated
Show resolved
Hide resolved
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.
We need to figure out what to do with zip
for collections versus cartesian builders.
val bb = iterator() | ||
val cc = c.iterator() | ||
val dd = d.iterator() | ||
|
||
val size = minOf( | ||
collectionSizeOrDefault(10), | ||
c.collectionSizeOrDefault(10), | ||
d.collectionSizeOrDefault(10) | ||
) | ||
val list = ArrayList<E>(size) | ||
while (bb.hasNext() && cc.hasNext() && dd.hasNext()) { | ||
list.add(transform(bb.next(), cc.next(), dd.next())) | ||
} | ||
return list |
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 impl was inspired by the Kotlin Std 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.
looks good!
"zip2" { | ||
forAll(Gen.nonEmptyList(Gen.int()), Gen.nonEmptyList(Gen.int())) { a, b -> | ||
val result = a.zip(b) | ||
val expected = a.all.zip(b.all).let(NonEmptyList.Companion::fromListUnsafe) |
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.
aren't we testing against our own zip version here?
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.
Yes, but that zip
is tested against Kotlin Std zip
in IterableTest
so it assumes our zip
is correct/tested. Which is added in this PR also.
Worked on this PR after requesting changes so dismissing my review.
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 folks!
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 PR updates/fixes some
zip
implementations and adds tests for it.It also deprecates
mapN
for collections, which were cartesian builders. For collections, this easily results in really big lists that make the JVM OOM.Example of dangerous usage:
This tries to create a list with 6^10 (60466176) elements and as a result OOMs. As you can see this easily results in bad code since here we're only using a list with 6 elements and it already results in problematic code.
Since this pattern isn't so common and is only safe with low arities we will recommend to simply use
flatMap/map
chains instead.