Add catchingExceptions method to replace runCatching
#4797
Conversation
|
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4797 +/- ##
===========================================
- Coverage 80.39% 80.39% -0.01%
===========================================
Files 2149 2149
Lines 56833 56838 +5
Branches 7131 7126 -5
===========================================
+ Hits 45693 45697 +4
+ Misses 8693 8690 -3
- Partials 2447 2451 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| * | ||
| * [Error]s are not caught by this function, as they are not meant to be caught in normal application flow. | ||
| */ | ||
| inline fun <T> catchingExceptions( |
There was a problem hiding this comment.
To be closer to runCatching, maybe rename to runCatchingExceptions?
Also I am wondering if we could still have a safe mode (for production?) where we still catch Errors. Here we take the risk to have crashes on production. Maybe we could have only this on debug build, so that we have time to fix existing problems?
There was a problem hiding this comment.
I don't think we can, not easily 🫤 . This is in a java library module so we don't have the concept of BuildConfig to check if it's a debug or release version and we can't inject the BuildMeta here either.
| * | ||
| * This is a safer version of [Result.mapCatching]. | ||
| */ | ||
| inline fun <R, T> Result<T>.mapCatchingException( |
There was a problem hiding this comment.
Maybe rename to mapCatchingExceptions? It seems that this is what you wanted to do according to the description of the detekt rule you wrote :)
| isSuccess -> catchingExceptions { block(getOrNull()!!) } | ||
| else -> Result.failure(exceptionOrNull()!!) | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe rewrite to
return fold(
onSuccess = { value -> catchingExceptions { block(value) } },
onFailure = { exception -> Result.failure(exception) }
)to avoid usage of !!.
| import org.matrix.rustcomponents.sdk.SessionVerificationController | ||
| import org.matrix.rustcomponents.sdk.SessionVerificationControllerDelegate | ||
|
|
||
| class FakeSessionVerificationController : SessionVerificationController(NoPointer) { |
There was a problem hiding this comment.
The (not-written) convention is to name this class FakeRustSessionVerificationController so that we see that it's not a regular Fake class that we use for Unit test.
It seems that I did not follow the convention for FakeQrCodeData, my bad...
| System.err.println(it) | ||
| } | ||
| exitProcess(1) | ||
| throw AssertionError(message) |
There was a problem hiding this comment.
The comment above can be removed I think!
This method creates a `Result` by catching only `Exception` throwables, also re-throwing `CancellationException` so coroutines can work as expected. Add also `Result.mapCatchingException` to replace `Result.mapCatching`, with the same behaviour of catching only exceptions.
With this we make sure no `Error` will be incorrectly caught and `CancellationException`s get to follow the coroutine cancellation flow.
…terparts. Also replace the instantiation of `Throwable` errors for tests with `RuntimeException`.
…sages of `runCatching`
…he cancellation exceptions and re-throw them after some cleanup
…rs those threw were previously silently ignored
# This is the 1st commit message: Rename `catchingExceptions` to `runCatchingExceptions` # The commit message #2 will be skipped: # fixup runcatching
jmartinesp
force-pushed
the
refactor/replace-runcatching
branch
from
45e2481 to
b9202d2
Compare
|
Content
catchingExceptions(block()): this method creates aResultby catching onlyExceptionthrowables, also re-throwingCancellationExceptionso coroutines can work as expected.Result.mapCatchingException(block())to replaceResult.mapCatching, with the same behaviour of catching only exceptions.tryOrNullalso catch only exceptions and re-throw cancellation exceptions.runCatchingandmapCatchingfrom being used.Motivation and context
Fixes #4778
Tests
The app builds, the tests should pass and we have Maestro for runtime.
Tested devices
Checklist