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

Change traverseEither/Validated to use buildList/Map #2368

Closed
wants to merge 5 commits into from
Closed

Change traverseEither/Validated to use buildList/Map #2368

wants to merge 5 commits into from

Conversation

kyay10
Copy link
Collaborator

@kyay10 kyay10 commented Apr 5, 2021

This also uses the list/map's size when available to prevent unnecessary array resizes in the middle of traversing. I also added 10 as the default for collectionSizeOrDefault because it seemed to be the de-facto all throughout the code.

(I just realized that I accidentally used Ctrl+Alt+L on the code which I think expanded some unrelated calls but oh well)

@kyay10 kyay10 mentioned this pull request Apr 5, 2021
@kyay10
Copy link
Collaborator Author

kyay10 commented Apr 5, 2021

It might be beneficial btw to add the tests from the Nel PR that make sure the head isn't traversed twice here just in case, but I'm not sure tbh.

@1Jajen1
Copy link
Member

1Jajen1 commented Apr 5, 2021

I'll use this, if we end up being fine with buildList, thanks!
However the diff looks quite weird, is this from the right branch?

It might be beneficial btw to add the tests from the Nel PR that make sure the head isn't traversed twice here just in case, but I'm not sure tbh.

I believe the new tests there cover those well enough as there are tests verifying the result lists including all elements. (They match against every traversed element and the initial lists)

@nomisRev
Copy link
Member

nomisRev commented Apr 6, 2021

I don't think we shouldn't add @OptIn(ExperimentalStdlibApi::class) to our APIs.
Only in some exceptional cases where we for example add overloads for kotlin.time.*, so that it doesn't hinder the user from using them.

I'm not sure when this is becoming stable, but I think we should post-pone this enhancement until then.

@raulraja raulraja closed this Apr 6, 2021
@raulraja raulraja deleted the branch arrow-kt:jo-fold-right-order April 6, 2021 09:19
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.

5 participants