michaelbull / kotlin-result

A multiplatform Result monad for modelling success or failure operations.
ISC License
1.02k stars 57 forks source link

Introduces throwIf and throwUnless #66

Closed grodin closed 2 years ago

grodin commented 2 years ago

Introduces the rethrow function as discussed in #64.

grodin commented 2 years ago

Having written this, I'm not convinced that it's actually all that ergonomic to use. Having to call it with two type parameters, despite the Ok parameter having no effect, makes it quite ugly IMHO.

There's also the fact that if you call it without specifying the type parameters, it does nothing and gives you no indication that it will do nothing. More detail in the kdoc comment would help that, but still makes it a bit unintuitive.

michaelbull commented 2 years ago

I agree regarding the necessity to explicitly type it, and the lack of effect when failing to do so is troublesome and will definitely catch people out. Wonder what other approaches we can think of.

Munzey commented 2 years ago

Two possible alternatives I was thinking about: 1) add another factory method runCatchingRethrowing<T> where type T is to be rethrown. I think this would cover most use cases.

2) An exclude<T>(excludeHandler){} scoped block that could specify a result of type T to be handled by exclude Handler. Certainly more of an odd way to do things, but it would let you exclude on both success and failure paths, as well as being more in line with this lib where Err can be of any type.

grodin commented 2 years ago

@Munzey the problem with both of those approaches is the same as my original problem:

any function taking Result<V, T> (including extension functions) has to be called with either zero or both type parameters.

E.g. runThrowingCatching would have to be called with

runThrowingCatching<String, CancellationException> {...

or whatever value and exception types you wanted.

It's the fact that you have to specify String there that I find annoying, because it's just noise. The value type is irrelevant to the functionality we want, and having to specify it is just an artifact of how Kotlin works.

grodin commented 2 years ago

@michaelbull I'll submit a PR for runSuspendCatching without this, and then if we find a way to make this PR work, we can rewrite runSuspendCatching in terms of rethrow() if it seems worth it.

grodin commented 2 years ago

Actually, I've just remembered another potential solution to this I'd thought of. I'll just sketch it here:

fun <V> Result<V, Throwable>.rethrowIf(
    predicate: (Throwable) -> Boolean
): Result<V, Throwable>

It's more general than rethrow as I wrote it originally in this PR but it might actually be more useful. This version definitely feels less focused and narrow in scope though.

Then we could write

runSuspendCatching( block: suspend () ->V) = 
    runCatching(block)
        .rethrowIf { it is CancellableException }
Munzey commented 2 years ago

could we avoid the noise of adding the exception to the generic params by just passing the kclass as parameter? Like

runCatchingRethrow(exceptionClazz: KClass<*>)

And just comparing Err.value::class with this?

Munzey commented 2 years ago

Another thought I just had regarding the semantics of rethrow - it doesnt really make sense outside of the context of runCatching. If I decide to create a result type with an exception in the failure case for some arbitrary logic, it doesnt mean anything to re throw as I was never going to throw to begin with. Perhaps just .throw or .throwIf is better if its an extension of result? runCatchingRethrowing would still make sense.

michaelbull commented 2 years ago

runCatchingRethrowing is quite the mouthful, and "catching" "rethrowing" seems like it would be far too hard to explain to callers

michaelbull commented 2 years ago

I like the rethrowIf - we could add rethrowUnless to be consistent with the stdlib, the same way we have toErrorIf, toErrorUnless.

I also think the comments made earlier about "throw" vs "rethrow" are worth considering. If we were never going to throw then "rethrow" doesn't make sense, so something like runCatching { blah}.throwIf { err is SQLException } would be where we are heading I think.

grodin commented 2 years ago

Finally got some time to work on this. I've changed 'rethrow' to 'throwIf' and implemented 'throwUnless' with tests for both.

michaelbull commented 2 years ago

Merged in 3b87373b238df613605cee0a82ee1e0def879507

Also added orElseThrow in f236e2674bf98f12f50fa740e1f70aa65b5464d5