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

chore: enable ruff lint rule TRY201 and B904 to improve raise stack traces #29166

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Jun 11, 2024

When intercepting and re-raising an error in python, it's almost always better to simply raise over raising the exception alias ie: raise ex. The main reason is that the stack trace often (always?) gets truncated in the process, losing track of where the error occurred.

Similarly, when intercepting and re-raising another exception, it's better to raise AnotherException() from ex (using from), so I'm fixing and enforcing this as well.

I found this while debugging and dealing with incomplete stack traces, and thought it be a good idea to fix and enforce using ruff's TRY201 and B904

@mistercrunch mistercrunch requested a review from a team as a code owner June 11, 2024 00:03
@github-actions github-actions bot added risk:db-migration PRs that require a DB migration api Related to the REST API labels Jun 11, 2024
When intercepting and re-raising an error in python, it's almost always better to simply `raise` over raising the exception alias ie: `raise ex`. The main reason is that the stack trace often (always?) gets truncated in the process, losing track of where the error occurred.

Similarly, when intercepting and re-raising another exception, it's better to `raise AnotherException() from ex`, so I'm fixing and enforcing this as well.

I found this while debugging and dealing with incomplete stack traces, and thought it be a good idea to fix and enforce using ruff's TRY201 and B904
@mistercrunch mistercrunch changed the title chore: enable ruff lint rule TRY201 chore: enable ruff lint rule TRY201 and B904 to improve raise stack traces Jun 11, 2024
@mistercrunch
Copy link
Member Author

Summoning some of our backend / python folks - @betodealmeida @dpgaspar @john-bodley , what do you think?

@giftig
Copy link
Contributor

giftig commented Jun 12, 2024

I think there might be cases where you legitimately don't want to use from because you don't care about the original exception, but it's probably not very often. But a bare raise is always better than reraising the original exception you just caught imo, for the reasons you mentioned.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL

@john-bodley john-bodley self-requested a review June 12, 2024 18:04
@mistercrunch mistercrunch merged commit 4bb2e2f into master Jun 12, 2024
42 checks passed
@mistercrunch
Copy link
Member Author

TIL

yeah I didn't know that until I was like "where's the rest of my stacktrace!?!?! how did we get here?"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the REST API preset-io risk:db-migration PRs that require a DB migration size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants