https://kotlinlang.org logo
Title
m

mitch

05/09/2021, 3:10 AM
@raulraja @simon.vergauwen I’ve raised this PR to address some (not all) of the missing option extensions that we had to introduce internally https://github.com/arrow-kt/arrow/pull/2397 if we can get these (esp. the traverses) to be supported natively in arrow, that’s going to be massive for us!
🙌 3
r

raulraja

05/09/2021, 9:18 AM
Looks great @mitch, thanks so much for taking the time 🙂
m

mitch

05/09/2021, 8:20 PM
Awesome thanks @raulraja, I've addressed your comments. Going to wait for others to review as well.
👏 2
p

pablisco

05/10/2021, 3:47 PM
Nice job 🙂 I have a question/thought for @raulraja and @simon.vergauwen. It’s already reviewed so may not warrant any changes. Some of the Option operations can be expressed as
nullableOp.toOption()
, would this be better or worse than duplicating logic? Duplicates aren’t always bad 😅 Let me know what you think 🙂
r

raulraja

05/10/2021, 3:58 PM
I think given we accepted Option back and people use it in places where nullable types can’t be used we should accept it as first class citizen and offer the same set of apis as other data types.
So it’s not so much to me a problem of duplication but consistency. If you as a user use Either, or nullable types and then at some point you have to use Option you’d expect the same set of available methods where posible so the API and coding experience is consistent.
p

pablisco

05/10/2021, 4:03 PM
Sorry @raulraja I think I didn’t explain myself well. I don’t mean to not have these but rather if we should, internally, define them as:
inline fun <C> traverseOption(
    fa: (B) -> Option<C>,
  ): Option<Either<A, C>> =
    traverse(fa).toOption()
or as extension functions: inline fun <A, B, C> Either<A, B>.traverseOption( fa: (B) -> Option<C>, ): Option<Either<A, C>> = traverse(fa).toOption() 🙂
r

raulraja

05/10/2021, 4:12 PM
I see, the general consensus was that whenever it’s possible regular members is favorable to users because it does not impose an additional import. Too many extension imports has been a problem to arrow and the only reason why we have in some cases extensions is due to the nature of contravariance and how kotlin treats it in members. For example it’s not possible to implement
Either.flatMap
as a member without using
@UnsafeVariance
or similar hacks but it’s possible to implement it as an extension because in extensions type arguments are invariant and spelled out in the function.
💯 1
p

pablisco

05/10/2021, 4:21 PM
Ignore the extension fun 😅 Shouldn’t have mixed matters hehe. The first snippet is the important one
r

raulraja

05/10/2021, 4:25 PM
you mean that in your example
traverse(fa)
returns a nullable value and the you convert it to option?
:yes: 1
if it follows the laws of the operation and it’s faster than whatever other way is implemented then yes. As a general rule of thumb for the impls we try to follow whatever is faster and still correct no matter what it takes.
p

pablisco

05/10/2021, 4:28 PM
So, to the end user would be the same but we save some code on our side. Nullable is “inlined” i.e. has no wrapper. But, as I said, it’s not always better to remove duplicates, depends on the case
All that said, LGTM 🙂