Skip to content
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

Change Exception to Throwable #73

Closed
wants to merge 1 commit into from
Closed

Change Exception to Throwable #73

wants to merge 1 commit into from

Conversation

seanghay
Copy link

Since Exception extends Throwable, I think the better way is to use Throwable over Exception for type and catching errors.

Kotlin Standard Library

image

Tweet

https://twitter.com/relizarov/status/1299248297849303040

Exception extends Throwable
@kittinunf
Copy link
Owner

@Globegitter what do you think about this though? If this is also something that we want to do I would love to do this for 4.0 version.

@bohsen
Copy link

bohsen commented Jan 12, 2021

This is good practice. Makes the usage safer.

@R4md4c
Copy link

R4md4c commented Jan 12, 2021

I doubt that's a good practice, as handling Throwable means you'd catch Error subclasses e.g. (OutOfMemoryError, VirtualMachineError or even AssertionError). I doubt that you'd want to catch an OutOfMemoryError since it points to an underlying problem in your code rather than a exception that you want to handle.

Also imagine you're doing an assertion that throws an AssertionError and you're expecting the code to crash if that assertion is not met, but in that case it won't crash since it will be swallowed by the Result monad, which for sure won't be a good thing and will result in some potentially long debugging time.

@bohsen
Copy link

bohsen commented Jan 12, 2021

If kittinunf/Result-library at some point turns into a multiplatform library this change is needed and also usage in serverside software would be safer with this.

Personal opinion:
If you don't catch it you don't handle it. Rethrow it if you choose not to handle it, but at least try to somehow log it.
And also only catch an exception if you have an idea of what to do with it. Use try-catch/runCatching with care.

@Globegitter
Copy link
Collaborator

Not sure if this would have any implications for existing code but it does sound to me like a good fix @kittinunf 👍

@kittinunf
Copy link
Owner

This should be superseded by #84

@kittinunf kittinunf closed this Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants