feat: Increase installation logging granularity and set process exit code properly#365
feat: Increase installation logging granularity and set process exit code properly#365laur89 wants to merge 3 commits intoReVanced:devfrom
Conversation
src/main/kotlin/app/revanced/cli/command/utility/InstallCommand.kt
Outdated
Show resolved
Hide resolved
| }.install(Installer.Apk(apk, packageName)) | ||
| } catch (e: Exception) { | ||
| logger.severe(e.toString()) | ||
| throw e |
There was a problem hiding this comment.
Why are we logging when throwing the exception which will be logged again?
There was a problem hiding this comment.
which will be logged again?
It won't be logged again. The err is rethrown only for the sake of control flow.
There was a problem hiding this comment.
Since this is a binary situation, you can use return logic.
There was a problem hiding this comment.
Think we should continue this discussion under the return code chain
| when (result) { | ||
| RootInstallerResult.FAILURE -> | ||
| RootInstallerResult.SUCCESS -> | ||
| logger.info("Mounted the APK file") |
There was a problem hiding this comment.
"Mounting" is an installation method. I think we don't need more granularity than "install" for both cases. Mentioning that it was mounted is not necessary too for the reason that the CLI input already mentions --mount as an installation method.
There was a problem hiding this comment.
'ite, both {RootInstallerResult.SUCCESS, AdbInstallerResult.Success} will log
Installed the APK file
There was a problem hiding this comment.
Happy w/
RootInstallerResult.FAILURE -> {
logger.severe("Failed to mount the APK file")or want it changed?
| is AdbInstallerResult.Failure -> | ||
| logger.severe(result.exception.toString()) | ||
| else -> | ||
| throw Exception() |
There was a problem hiding this comment.
raising an empty exception doesn't look right to me.
There was a problem hiding this comment.
Same as explained above - it's for control flow only.
There was a problem hiding this comment.
In this case, you can probably just return?
There was a problem hiding this comment.
Like above, we should continue this discussion under the return code chain
src/main/kotlin/app/revanced/cli/command/utility/InstallCommand.kt
Outdated
Show resolved
Hide resolved
|
|
||
| runBlocking { | ||
| deviceSerials?.map { async { install(it) } }?.awaitAll() ?: install() | ||
| try { |
There was a problem hiding this comment.
I generally find exceptions ugly/messy code. They introduce nesting and unnecessary blocks of code. Using return values seems simpler for "binary" cases. Exceptions have particular use case for raising messages or multiple kinds of cases not just true or false.
There was a problem hiding this comment.
If you pefer not to use exceptions for this control flow, then what do you propose?
Perhaps change InstallCommand/UninstallCommand to implement Callable instead of Runnable and return code (i.e. Int) from install() / uninstall()?
There was a problem hiding this comment.
Is this the expected way of returning an exit code for PicoCLI? If so, every subcommand should be subject to this change.
There was a problem hiding this comment.
Honestly no idea what the common there pattern is.
|
|
||
| when (result) { | ||
| RootInstallerResult.FAILURE -> | ||
| RootInstallerResult.SUCCESS -> |
There was a problem hiding this comment.
I am just noticing code duplication here and in the patch subcommand, possibly extracting into a common function makes sense.
oSumAtrIX
changed the title
|
If we introduce exit code, then it should probably be introduced for more scenarios like failing to patch, failing to connect to the device, failing to install or other types of failures consistently, in another PR. |
src/main/kotlin/app/revanced/cli/command/utility/InstallCommand.kt
Outdated
Show resolved
Hide resolved
laur89
force-pushed
the
improve-install-uninstall-logging
branch
2 times, most recently
from
a7de7a0 to
7ce78b7
Compare
- handle {Root,Adb}InstallerResult in InstallCommand & UninstallCommand.
this is a non-functional change, only aiming to improve the logged
feedback to the user
- fixes ReVanced#362
- exit InstallCommand/UninstallCommand with exit code 1 on detected command failures or thrown exceptions; log where appropriate - fixes ReVanced#362
laur89
force-pushed
the
improve-install-uninstall-logging
branch
from
7ce78b7 to
830bd97
Compare
2 participants
No description provided.