solrudev / Ackpine

Android package installer library
https://solrudev.github.io/Ackpine/
Apache License 2.0
51 stars 7 forks source link

Exceptions and Typed errors #67

Open nvkleban opened 1 week ago

nvkleban commented 1 week ago

Hi! Thank you for the library, it's really helpful.

While investigating it I found that it's requires wrapping packageInstaller.createSession(apkUri).await() in try\catch block. Have you considered instead of throwing exceptions to pass typed errors instead as you already do, but for all other cases too? It helps with understanding what could happen and react to it before it happens.

For me as user of library I can't really distinguish between InstallFailure.Exceptional and Exception been thrown. One example would be - I encountered FileNotExists exception (when apk file was missing), while I expected some sort of InstallFailure

solrudev commented 1 week ago

@nvkleban Thanks for an interesting question! This is quite complicated topic. In my opinion, as we're dealing with coroutines, await() function should rethrow all exceptions encountered, because this is an expected behavior by convention. Thus, actually, you'll never get InstallFailure.Exceptional as a return value.

However, as you noted, this is quite confusing. This peculiarity stems from core architectural choice of designing Session as an observable object to not be dependent on a concrete asynchrony implementation baked into frameworks or a language. I couldn't make Exceptional failure to not be a part of SessionResult.Error failure sealed hierarchy and instead to be a part of a separate one and maintain the same types for failures across modules. Sadly, I don't think Kotlin's type system allows this. Taking into consideration what I wrote in the first paragraph, I'm reluctant to introduce such a change. As a workaround, you can write your own await() wrapper which will catch exceptions and return them as InstallFailure.Exceptional objects, though I'll refrain from providing such functionality in the library.

Perhaps, the best compromise is to introduce a separate sealed hierarchy of failures specifically for coroutines, like I suggested above, though it'll be a major API change. What do you think?