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

Deprecate Iterable#unzip in favor of stdlib method #3352

Merged
merged 3 commits into from
Jan 19, 2024

Conversation

terminalnode
Copy link
Contributor

From what I can tell the other unzip methods have no equivalent in the stdlib.

NonEmptyList is of course covered by the Iterable#unzip, but still has a use-case because of returning a pair of NonEmptyLists rather than a pair of regular Lists.

This is Iterable#unzip in kotlin 1.9.22:

public fun <T, R> Iterable<Pair<T, R>>.unzip(): Pair<List<T>, List<R>> {
    val expectedSize = collectionSizeOrDefault(10)
    val listT = ArrayList<T>(expectedSize)
    val listR = ArrayList<R>(expectedSize)
    for (pair in this) {
        listT.add(pair.first)
        listR.add(pair.second)
    }
    return listT to listR
}

@Deprecated(
"Unzip is being deprecated in favor of the standard library version.\n$NicheAPI",
ReplaceWith("unzip()", "kotlin.collections.unzip")
)
public fun <A, B> Iterable<Pair<A, B>>.unzip(): Pair<List<A>, List<B>> =
fold(emptyList<A>() to emptyList()) { (l, r), x ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should just delegate to stdlib unzip

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to the deprecation you mean? Changed this in e81b318, not sure if there's a better way to call the standard library method than using an import alias though (to avoid confusion with the other unzip methods in the file).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that's exactly what I intended! I think an import alias is just the right approach here. I wonder how the ReplaceWith will function because of the name discrepancy though. kotlin.collections.* is a default import, and so perhaps we could just deprecate this with HIDDEN level. I'd rather wait though on another reviewer's opinion as to whether HIDDEN is the right approach here or not.

@kyay10
Copy link
Collaborator

kyay10 commented Jan 18, 2024

NonEmptyList is of course covered by the Iterable#unzip, but still has a use-case because of returning a pair of NonEmptyLists rather than a pair of regular Lists.

Perhaps the implementation for NonEmptyList should use stdlib's unzip and then call toNonEmptyListOrNull()!!. You can do that in this PR if you want, but it's not necessary for now.

From what I can tell the other unzip methods have no equivalent in the
stdlib.

NonEmptyList is of course covered by the Iterable#unzip, but still has
a use-case because of returning a pair of NonEmptyLists rather than
a pair of regular Lists.
@terminalnode
Copy link
Contributor Author

NonEmptyList is of course covered by the Iterable#unzip, but still has a use-case because of returning a pair of NonEmptyLists rather than a pair of regular Lists.

Perhaps the implementation for NonEmptyList should use stdlib's unzip and then call toNonEmptyListOrNull()!!. You can do that in this PR if you want, but it's not necessary for now.

Added this in 484e3df. I can't run all the tests locally for some reason (getting some random failures even with the main branch), but I think all the relevant tests are passing.

@kyay10
Copy link
Collaborator

kyay10 commented Jan 18, 2024

The PR checks will take care of trying the tests, don't worry!

@nomisRev
Copy link
Member

I can't run all the tests locally for some reason (getting some random failures even with the main branch)

Might be KMP setup, like @kyay10 said, not important since CI verifies everything but if you run into this again in the future KDoctor is a great tool to help you setup your KMP development environment.

Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @terminalnode! 🙌

@nomisRev nomisRev merged commit 64b0e54 into arrow-kt:main Jan 19, 2024
10 checks passed
@terminalnode terminalnode deleted the deprecate-iterable-unzip branch January 19, 2024 12:35
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.

3 participants