-
Notifications
You must be signed in to change notification settings - Fork 451
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 partitionMap to Iterable (#3001) #3004
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.
PR looks perfect thank you for your contribution @jsoizo 🙌 Congrats on your first contribution 🥳
* <!--- KNIT example-iterable-20.kt --> | ||
* <!--- TEST lines.isEmpty() --> | ||
*/ | ||
public inline fun <T, A, B> Iterable<T>.partitionMap(f: (T) -> Either<A, B>): Pair<List<A>, List<B>> { |
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.
Wondering about the name 🤔 I asked chat-gpt 😁 It suggested seperateMap
, it would be more aligned with seperateEither
. What do you think?
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.
@nomisRev I didn't have a particularly strong opinion on the naming of this function, and thought it would be fine as it is in Scala's partitionMap.
As you mention in the Kotlin slack, I prefer to assume that Iterable<Either<A,B>>.separateEither
overloads this. I would change it that way.
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.
I have no strong opinion towards any names, but I just feel that they should be aligned. I somehow missed that Kotlin Std already has partition
.
Haskell names seperateEither
as partitionEither
so in that case partitionEither
and partitionMap
would make sense. I invited some other reviewers to the PR.
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 to me. Thanks for your contribution, @jsoizo!
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 great!
Thanks @jsoizo !!!
Implementation for #3001