Validated conditionally #2603
Validated conditionally #2603
Conversation
| public inline fun <L, R> conditionally(test: Boolean, ifFalse: () -> L, ifTrue: () -> R): Validated<L, R> = | ||
| if (test) Valid(ifTrue()) else Invalid(ifFalse()) |
There was a problem hiding this comment.
Seeing this version for Validated makes me wondering if it's more clear or obscuring the code.
All snippets below are equivalent, since Arrow is considered to have a high learning curve think we should be weary of the API surface in Arrow.
val guard = false
val a = Validated(guard, { "succeed" }) { "fail" }
val b = if(guard) Valid("succeed") else Invalid("fail")
val c = Either.conditionally(guard, { "succeed" }) { "fail" }
val d = if(guard) "succeed".right() else Invalid("fail").left()
val e = either<String, String> {
ensure(guard) { "fail" )
"succeed"
}
There was a problem hiding this comment.
I've sometimes used this, and I think it gains when you write it like this:
val c = Either.conditionally(
guard,
ifFalse = { "fail " },
ifTrue = { "succeed" }
)Given that similar function is available in Scala, my feeling is that this may actually help users.
There was a problem hiding this comment.
What should be our decision here, @nomisRev? I am happy with approval.
There was a problem hiding this comment.
I am also in favor of less obscure code, one can still bridge to Validated via toValidated, bc then we probably need to do the same for Option and other Bifunctors.
But this involves a further look into Arrow, which API's add obscure code paths in general.
There was a problem hiding this comment.
@abendt I think these functions are fine but they increase the API surface which is something we want to trim going forward.
if (test) Valid(ifTrue()) else Invalid(ifFalse())vs
Validated.conditionally(test, ifFalse(), ifTrue())We were in favor of these kinds of idioms in the past but since we decided to complement the kotlin std lib and the lang, we prefer to use the lang control flow mechanism where they are expression-based as in the case of if. An exception for this is Either.catch because control of exceptions and the semantics with suspending are harder for users to encode.
Happy with this going forward since this PR has already two approvals.
Going forward I think we should look into trimming down all data types APIs where possible.
IMO most data types like Either can get away with just fold and bind and the rest are largely irrelevant because of bind and the either block. Even more irrelevant in the future in the case of multiple context receivers where we can constraint functions to the EffectScope<E> and just shift without using Either then interpret to Either or Validated. For example as shown in #2661
class DomainError(msg: String)
context(CoroutineScope, EffectScope<DomainError>)
suspend fun program(): Int {
val fa = async<Int> { shift(DomainError("error")) }
val fb = async { delay(2000); 1 }
return fa.await() + fb.await()
}There was a problem hiding this comment.
Happy with this going forward since this PR has already two approvals.
I actually removed my approval (sorry @abendt), since we have a lot of issues with the API surface of Arrow.
I think if (test) Valid(ifTrue()) else Invalid(ifFalse()) reads better than Validated.conditionally(test, ifFalse(), ifTrue()) since everyone understands the first one, but the same isn't true for the second one. So this actually increases the learning curve of Arrow, which definitely is a bad thing. This kind of idioms exists in languages/effect systems that use final tagless and express ifM, conditionally through tagless implementations.
Therefore I'm actually in favor of deprecation Either.conditionally instead of addding Validated.conditionally as I argued in another comment above too.
What do you think @abendt?
There was a problem hiding this comment.
fair enough @nomisRev and @raulraja
i can fully understand the reasoning to simplify the API surface, makes sense for maintainability and as you mentioned for the learning curve.
my main motivation for the PR was to write and contribute some code. So i am not too attached to the actual functionality, so i fully support your decision.
my only suggestion would be to cleanup the existing github issues so its easier to find the ones you want support on.
There was a problem hiding this comment.
Thank you for your understanding and support of Arrow @abendt. We should really make a better effort in maintaining the Github issues but it's very difficult and time-consuming. Shameful to admit it's been on my TODO list for 2+ years to clean up and add beginner tickets :/
this adds Validated.conditionally and conditionallyNel (#2570)