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
base: main
Are you sure you want to change the base?
Conversation
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.
4fbd274
to
464fedb
Compare
sanic/exceptions.py
Outdated
@@ -86,6 +87,7 @@ def __init__( | |||
self.status_code = status_code or self.status_code | |||
self.quiet = quiet | |||
self.headers = headers | |||
self.message = message # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a type error here — it looks like message
the argument of __init__
can take a bytes
object, but the class attribute can only be a str
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect the option for it to be bytes
might be because of STATUS_CODES
messages being in bytes (assuming someone passed one of those to init), and this may be necessary to keep for compatibility.
Can you get away with typing without the ignore if you add some logic to decode the argument before storing to self.message
, if bytes
was received?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that makes sense! I'll update my PR to do that :)
sanic/exceptions.py
Outdated
@@ -86,6 +87,7 @@ def __init__( | |||
self.status_code = status_code or self.status_code | |||
self.quiet = quiet | |||
self.headers = headers | |||
self.message = message # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect the option for it to be bytes
might be because of STATUS_CODES
messages being in bytes (assuming someone passed one of those to init), and this may be necessary to keep for compatibility.
Can you get away with typing without the ignore if you add some logic to decode the argument before storing to self.message
, if bytes
was received?
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.
|
There was a problem hiding this 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!
Thanks for reviewing so quickly! I followed the pattern of the existing code to use |
This allows subclasses of SanicException to access their message via the
message
attribute. This makes it consistent with thestatus_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 orstr()
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