-
Notifications
You must be signed in to change notification settings - Fork 449
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
Simplify recover, and catch APIs. Add getOrElse #2954
Conversation
} | ||
return catch(action) { t: Throwable -> if (t is T) catch(t) else throw t } | ||
} | ||
public inline fun <reified T : Throwable, A> catch(action: () -> A, catch: (T) -> A): A = |
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.
This also what we usedand needed in Gh Alerts Project to wrap side-effect APIs like PSQLException
-> UserAlreadyExists
and would allow easily writing type-safe integration libraries for JDBC
or PSQL
.
Wondering if this should be on Raise
companion similar to Either.catch
🤔 They're basically the duality of each-other. Similarly, does Either.catch
need a reified
variant?
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.
Raise sort of feels like much more of a primitive than Either, and thus it feels like catch
makes more sense than Raise.catch
. The name Raise.catch
also just feels weird from a language standpoint
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.
Raise sort of feels like much more of a primitive than Either
It absolutely is!
The name Raise.catch also just feels weird from a language standpoint
Thanks for the feedback catch
is also applicable outside of Raise
, so that makes sense!
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.
Amazing simplification!
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 @nomisRev !
Merging this PR, all feedback and suggestions are still very welcome 🙏 |
Draft changes related to #2948 to facilitate the discussion. Also closes #2953
cc\ @freynder
Description about changes (copied from comments in #2948)
The lambda
Raise<R>.() -> A
allows to bind, or unwrap,Option
and in the caseNone
is encountered it will invoke the lambda. The lambda allows to raise a value ofR
into the outerinterface Raise<R>
, or fallback to a value of A.Raise<R>
lambda receiver is redundant because we're already inside the context ofinterface Raise<R>
, so even without the lambda receiver we can callraise(r)
.Raise<R>
, we have() -> A
which matches the already existingOption
API ofgetOrElse
.interface
, it cannot beinline
so it's inferior to the already existinggetOrElse
which is inline and thus allows forsuspend
to pass through.So this API can be completely replaced by
getOrElse
and simplifies the examples of the KDoc above.As the example shows we can now call
suspend fun delay
directly into thegetOrElse
lambda, and we can also still callraise
& co from inside thegetOrElse
lambda.The same applies for
Result#bind
. Since none of this code has yet been released, we can still remove these APIs in favour of the simplerinline
methods on the original APIs. Fixing this issue also requires some other small changes in Builders.kt.There is multiple benefits to this actually, and we should review the current signatures in
arrow.core.raise.*
to see how we can simplify them.Looking for example at
recover
:Besides still supporting the current use-cases it also makes this function more general purpose, because you can now use it to execute
Raise<E>
based programs and resolve the error completely without having to stick toRaise<E>
.