Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Replace Option<T> with ? in ListK (part 1) #196

Merged
merged 16 commits into from
Aug 12, 2020

Conversation

tapegram
Copy link
Contributor

@tapegram tapegram commented Jul 24, 2020

Part 1 of providing alternatives for fns that use Option for ListK.

Ran into https://discuss.kotlinlang.org/t/overloaded-function-with-lambda-parameter-of-different-return-types/6053/2 when creating a nullable version of filterMap, so I gave the nullable one a different and less good name :(

Would especial appreciate any feedback on the naming of these functions, since the older Option based functions already have the good names and the type system struggles to handle overloading with generics or lambdas without requiring explicit typing by the consumer, so I had to make up new names when required.

@nomisRev
Copy link
Member

Oh, that sucks :( This should be fixable with @BuilderInference, but that's still experimemtal isn't it?

@tapegram tapegram marked this pull request as ready for review July 31, 2020 13:00
@tapegram tapegram changed the title Add nullable fns to replace option fns 2 Replace Option<T> with ? in ListK (part 1) Jul 31, 2020
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.

The namings used here look good to me! 👍

I see a lot of the names involve recent contributions by @1Jajen1 and @abendt.

* }
* ```
*/
fun <B> filterNullMap(f: (A) -> B?): ListK<B> =
Copy link
Member

Choose a reason for hiding this comment

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

Should we do? We're also trying to algining more to the names familiar in Kotlin from the std. IIRC there is 2 Iterable extensions for the signature in the std. filterMap and mapNotNull?

@JvmName("mapNotNull")
fun <B> filterMap(f: (A) -> B?): ListK<B>

Copy link
Member

Choose a reason for hiding this comment

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

I think as long as the stdlib ones don't have any obvious drawbacks we shouldn't have duplicates names and signatures.

Copy link
Member

Choose a reason for hiding this comment

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

In that case I think we should name it mapNotNull as Kotlin's Std?

We can re-introduce filterMap if we want later on.

@nomisRev nomisRev requested a review from 1Jajen1 August 3, 2020 08:58
Copy link
Member

@1Jajen1 1Jajen1 left a comment

Choose a reason for hiding this comment

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

👍

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.

I think filterNullMap should renamed to mapNotNull to be inline with Kotlin's Std.

Thanks for the contribution @tapegram! 🙌 🎉

* }
* ```
*/
fun <B> filterNullMap(f: (A) -> B?): ListK<B> =
Copy link
Member

Choose a reason for hiding this comment

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

In that case I think we should name it mapNotNull as Kotlin's Std?

We can re-introduce filterMap if we want later on.

Copy link
Member

@aballano aballano left a comment

Choose a reason for hiding this comment

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

Just some nits, overall looks good!

arrow-core-data/src/main/kotlin/arrow/core/ListK.kt Outdated Show resolved Hide resolved
arrow-core-data/src/main/kotlin/arrow/core/ListK.kt Outdated Show resolved Hide resolved
arrow-core-data/src/main/kotlin/arrow/core/ListK.kt Outdated Show resolved Hide resolved
Tavish Pegram and others added 3 commits August 10, 2020 14:36
Co-authored-by: Alberto Ballano <aballano@users.noreply.github.com>
Co-authored-by: Alberto Ballano <aballano@users.noreply.github.com>
Co-authored-by: Alberto Ballano <aballano@users.noreply.github.com>
@nomisRev
Copy link
Member

Thanks so much @tapegram for the contribution!

@nomisRev nomisRev merged commit 6eff086 into arrow-kt:master Aug 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants