Skip to content

Conversation

@nicolassanmar
Copy link
Contributor

@nicolassanmar nicolassanmar commented Jun 23, 2023

Exceptions with detail but no message add that property to the exception value.

Fixes #2192

Exceptions with `detail` but no `message` add that property to the exception value.

Fixes getsentryGH-2192
@nicolassanmar nicolassanmar changed the title Master fix: Exceptions include detail property for their value Jun 23, 2023
Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

@nicolassanmar Thanks a ton for this PR! The change looks good, left a small comment. Could you please also add a testcase or two for this?

@nicolassanmar
Copy link
Contributor Author

@nicolassanmar Thanks a ton for this PR! The change looks good, left a small comment. Could you please also add a testcase or two for this?

Sure, been busy this week, but I will update this when I can

@goharShoukat
Copy link
Contributor

@nicolassanmar Thanks a ton for this PR! The change looks good, left a small comment. Could you please also add a testcase or two for this?

Sure, been busy this week, but I will update this when I can

@sentrivana I have added test to this PR.

For some reason, the new commit doesn't show up here and shows on a new branch.
#2294

@sentrivana
Copy link
Contributor

Thanks @goharShoukat! I'll take a look sometime this week.

@sentrivana sentrivana merged commit 2f14816 into getsentry:master Aug 10, 2023
@sentrivana
Copy link
Contributor

@nicolassanmar @goharShoukat Thank you both!

@goharShoukat
Copy link
Contributor

Been following the changes you made Ivana. I had a question. The tests I wrote worked in Python 3.10. Is the attempt here to make the code backward compatible too?

This is my first every open-source contribution, albeit a very small one, I am still quite proud to make it. Looking forward to contributing in the future.

@sentrivana
Copy link
Contributor

@goharShoukat It's a great first contribution! Congrats! :)

The change I made to the test had to do with the change I made to the behavior of the get_error_message function (this line specifically https://github.com/getsentry/sentry-python/pull/2193/files#diff-e9f88ca6368ce622839e078ed26c741a927752c01ceae64cd504d3ae86de6fd2R689). Before this PR, we were falling back to safe_str(exc_value) in case we didn't succeed in getting the message attribute, but in the previous version of this PR we would just return an empty string if we didn't find neither message nor detail, which would have been a breaking change from before. I then had to adjust the test to also check that the fallback is safe_str(exc_value) and not an empty string.

Hope that makes sense. Looking forward to more contributions from you in the future!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

FastAPI's HTTPExceptions do not get sent with the error message as subtitle

4 participants