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

Added high arity for arrow-core.api functions #3360

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

yesitskev
Copy link
Contributor

  • Added higher arity for some functions in arrow-core.
    • NonEmptyLis.zip
    • Sequence.zip
    • Map.zip
    • Iterable.zip
    • memoize() + MemoizeKeys
    • Raise<NonEmptyList>.zipOrAccumulate

Certain private interfaces and objects had to be made public to accommodate these additions:

  • Iterable.collectionSizeOrDefault(default: Int)
  • interface MemoizedCall<in F, out R>

@serras
Copy link
Member

serras commented Jan 25, 2024

We actually made the decision long time ago to "cut" these APIs (andd Tuple) to 10 parameters.

@nomisRev
Copy link
Member

nomisRev commented Jan 25, 2024

@serras this introduces a new module. The decision was made for binary size reasons rather than maintenance. Maintenance wise this code is not very expensive.

Since we regularly get such request, I personally feel okay with having a seperate opt-in module for those that want higher arities.

Although, it might need to be done with a warning because I feel that Kotlin doesn't promote this code style. If you have 9+ parameters it's probably a good indication there is a need to split data into smaller entities.

@serras
Copy link
Member

serras commented Jan 26, 2024

Oh, I didn't realize that! Then, of course, this is more than welcome :)

My only suggestion would be not to expose the private functions from arrow-core, since they are really implementation details. Maybe the memoization support is not so important? Since also in arrow-2 some of those functions are now in arrow-functions instead...

@serras serras mentioned this pull request Jan 30, 2024
22 tasks
@serras
Copy link
Member

serras commented Jan 30, 2024

@yesitskev is it OK if I go forward and remove the memoize higher-arity versions, so that we don't have to expose the internals? Anyway those APIs have been superseded by MemoizedDeepRecursiveFunction, so they are not so relevant anymore.

@yesitskev
Copy link
Contributor Author

@serras that's perfect. Feel free to prune whichever is necessary 👍

@serras
Copy link
Member

serras commented Feb 2, 2024

I've removed the memoization part, which required us exposing the internals of memoize. Once the build is green, I'll proceed to merge into arrow-2 and update the docs.

@serras serras merged commit 039a1c9 into arrow-kt:main Feb 6, 2024
10 checks passed
@yesitskev yesitskev deleted the kw/core-high-arity branch February 13, 2024 07:17
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