Improve re-raises of Exceptions #4142
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When we use try-except in our code base, often we raise a new exception to replace the original exception. It is good practice to either wrap around the original exception (`from exc`), or bin the original exception (`from None`) if the new exception is a self-contained description of what happened. The latter is for instance the case when we wrap around a dict, catch the KeyError, and provide more meaningful Exception with message, as the `KeyError` is not what the user is after, it wants to know that dtype1 cannot be converted to dtype2, etc. Without re-raise, you will see `During handling of the above exception, another exception occurred`, which means the two exceptions, original and new, are treated unrelated. With `from exc`, you will see `The above exception was the direct cause of the following exception:`. With `from None`, you will get a clean traceback without the original exception. Found all cases using pylint (`pylint polars --disable=all --enable=raise-missing-from`). Discussion on the rollout of pylint in CI takes place in pola-rs#4044. Background reading: https://stefan.sofa-rockers.org/2020/10/28/raise-from/
Codecov Report
@@ Coverage Diff @@
## master #4142 +/- ##
==========================================
+ Coverage 58.42% 58.43% +0.01%
==========================================
Files 436 436
Lines 66888 66888
==========================================
+ Hits 39077 39086 +9
+ Misses 27811 27802 -9
Continue to review full report at Codecov.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we use try-except in our code base, often we raise a new exception to replace the original exception. It is good practice to either wrap around the original exception (
from exc), or bin the original exception (from None) if the new exception is a self-contained description of what happened. The latter is for instance the case when we wrap around a dict, catch the KeyError, and provide more meaningful Exception with message, as theKeyErroris not what the user is after, it wants to know that dtype1 cannot be converted to dtype2, etc.Without re-raise, you will see
During handling of the above exception, another exception occurred, which means the two exceptions, original and new, are treated unrelated. Withfrom exc, you will seeThe above exception was the direct cause of the following exception:. Withfrom None, you will get a clean traceback without the original exception.Found all cases using pylint (
pylint polars --disable=all --enable=raise-missing-from). Discussion on the potential addition of pylint in CI takes place in #4044.Background reading: https://stefan.sofa-rockers.org/2020/10/28/raise-from/