-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Standardize init of exceptions #2545
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2545 +/- ##
=============================================
+ Coverage 88.617% 88.783% +0.165%
=============================================
Files 87 87
Lines 6844 6865 +21
Branches 1178 1179 +1
=============================================
+ Hits 6065 6095 +30
+ Misses 537 530 -7
+ Partials 242 240 -2
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
I am a bit torn. On the one hand: we have exceptions that are a bit of a mess. There is not a consistent On the other: exceptions are very fundamental. Changing them will break for users. I thought about ways to solve this:
Neither of these solutions will work, and have their own problems. Anyone else have some thoughts? Right now I am leaning towards fixing what we can, and leaving the breaking changes (a uniform constructor) for another time. |
I generally stand with "break it now to fix all of the future" (as opposed to keeping broken APIs for compatibility). But yes, at a bare minimum one should be able to write code that then works both with old and new Sanic versions, even if it requires changes to some old code. If it is about kwargs' names being changed, I suppose both names could be supported on init functions as a deprecation path. Removal of positional args is harder but at least one can then always fix the code to use kwargs instead and it will work with older Sanic too. Not a big fan of either one of the two solutions that you suggest, as both would increase complexity and confusion quite a bit. |
Yup. Which is why I ruled them out. The specific issue is this: class SanicException(Exception):
def __init__(
self,
message: Optional[Union[str, bytes]] = None,
status_code: Optional[int] = None,
quiet: Optional[bool] = None,
context: Optional[Dict[str, Any]] = None,
extra: Optional[Dict[str, Any]] = None,
) -> None:
...
class MethodNotAllowed(SanicException):
def __init__(self, message, method, allowed_methods):
...
class Unauthorized(SanicException):
def __init__(self, message, status_code=None, scheme=None, **kwargs):
... We have subclasses with incompatible signatures. I would really like to get to to keyword args (except class SanicException(Exception):
def __init__(
self,
message: Optional[Union[str, bytes]] = None,
*,
status_code: Optional[int] = None,
quiet: Optional[bool] = None,
context: Optional[Dict[str, Any]] = None,
extra: Optional[Dict[str, Any]] = None,
) -> None:
...
class MethodNotAllowed(SanicException):
def __init__(
self,
message,
method,
allowed_methods
*,
status_code: Optional[int] = None,
quiet: Optional[bool] = None,
context: Optional[Dict[str, Any]] = None,
extra: Optional[Dict[str, Any]] = None,
):
...
class Unauthorized(SanicException):
def __init__(
self,
message,
status_code=None,
scheme=None
*,
status_code: Optional[int] = None,
quiet: Optional[bool] = None,
context: Optional[Dict[str, Any]] = None,
extra: Optional[Dict[str, Any]] = None,
):
... So, yes, the issue is we have bad subclassing that messes up positional args. |
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. Message taking bytes is a bit odd (Exception only takes str) but other than that no complaints.
Not sure when or why that was added, but I think its been there a while. |
Needs some more cleanup. But this is an attempt to make raising exceptions a more consistent experience. It will be breaking however. There are a few existing exceptions that define their own
__init__
with a signature that does not match. To the extent we can preserve those orders, I will. But, I think it will be best to convert the other arguments (status_code
,extra
,context
,headers
) to keyword.Closes #2601