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

Make the .message field on exceptions non-empty #2947

Merged
merged 8 commits into from
Jun 22, 2024

Conversation

ekzhang
Copy link
Contributor

@ekzhang ekzhang commented May 6, 2024

This allows subclasses of SanicException to access their message via the message attribute. This makes it consistent with the status_code, quiet, headers attributes that were previously present on the class.

Currently, the message attribute is present on the class but not on the instance, so accessing SanicException("Failed!").message will return an empty string "" instead of the actual message "Failed!", which you can get by calling the __str__() method or str() function.

This is a bit surprising, since the .message attribute shows up in autocomplete and type-checking. It also happens for the other exceptions, like BadRequest().message == "" as well.

I set the message attribute on instances of SanicException and added tests for this new behavior.

Doc reference

https://sanic.dev/en/guide/best-practices/exceptions.html#exception-properties

@ekzhang ekzhang requested review from a team as code owners May 6, 2024 03:24
This allows subclasses of SanicException to access their message via the `message` attribute. This makes it consistent with the `status_code`, `quiet`, `headers` attributes that were previously present on the class.

Currently, the message attribute is present on the class but not on the instance, so accessing SanicException().message will return an empty string "" instead of the actual message "Internal Server Error", which you can get by calling the __str__() method or str().

This is a bit surprising, since the .message attribute shows up in autocomplete and type-checking. It also happens for the other exceptions, like str(BadRequest()) == "" as well.

I set the message attribute on instances of SanicException and added tests for this new behavior.
@ekzhang ekzhang force-pushed the ekzhang/sanic-message-attribute branch from 4fbd274 to 464fedb Compare May 6, 2024 03:31
sanic/exceptions.py Outdated Show resolved Hide resolved
sanic/exceptions.py Outdated Show resolved Hide resolved
@ekzhang
Copy link
Contributor Author

ekzhang commented May 6, 2024

Hi @Tronic, I updated the constructor to decode bytes arguments to str. It is a bit more verbose but works without disabling the type-checker on that line now.

I also updated the test!


The current CI failures appear to be from an unrelated error to this changeset.

sanic/request/types.py:89: error: "ctx_type" cannot appear after "sanic_type" in type parameter list because it has no default type  [misc]

Tronic
Tronic previously approved these changes May 6, 2024
Copy link
Member

@Tronic Tronic left a comment

Choose a reason for hiding this comment

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

LGTM, if not barring minor cleanup (e.g. removal of "utf8" argument on decode, perhaps some restructuring). I will assume @ahopkins to do another review before merging; from my point this is approved as is or with changes. Thanks for the contribution!

@ekzhang
Copy link
Contributor Author

ekzhang commented May 7, 2024

if not barring minor cleanup (e.g. removal of "utf8" argument on decode, perhaps some restructuring).

Thanks for reviewing so quickly! I followed the pattern of the existing code to use .decode("utf8") but I'm also happy to amend that call or restructure anything.

@ahopkins
Copy link
Member

@ekzhang I made some changes to the PR to keep the logic closer to the original intent to handle some of the border cases that your implementation may have broken. Thanks for the PR, and I certainly appreciate the additional tests you added.

@ahopkins ahopkins merged commit 853f831 into sanic-org:main Jun 22, 2024
25 checks passed
@ekzhang ekzhang deleted the ekzhang/sanic-message-attribute branch June 22, 2024 22:57
@ekzhang
Copy link
Contributor Author

ekzhang commented Jun 26, 2024

That's amazing, thank you!!

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

3 participants