Standardize init of exceptions

[ahopkins]

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

[codecov]

Codecov Report

Patch coverage: 92.187% and project coverage change: +0.165 🎉

Comparison is base (a245ab3) 88.617% compared to head (a495a31) 88.783%.

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     

Impacted Files	Coverage Δ
sanic/server/protocols/websocket_protocol.py	71.428% <33.333%> (ø)
sanic/exceptions.py	97.247% <94.000%> (-1.700%)	⬇️
sanic/__init__.py	100.000% <100.000%> (ø)
sanic/app.py	90.102% <100.000%> (+0.033%)	⬆️
sanic/handlers/content_range.py	100.000% <100.000%> (ø)
sanic/worker/loader.py	94.666% <100.000%> (+3.117%)	⬆️
sanic/worker/serve.py	98.571% <100.000%> (+7.142%)	⬆️

... 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ahopkins]

I am a bit torn.

On the one hand: we have exceptions that are a bit of a mess. There is not a consistent __init__, and properties are not guaranteed to be there.

On the other: exceptions are very fundamental. Changing them will break for users.

---

I thought about ways to solve this:

1. monkeypatch until we can deprecate
2. providing a legacy and a new module

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.

[Tronic]

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.

[ahopkins]

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 message), and allow the subtypes to add their own.

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.

[Tronic]

LGTM. Message taking bytes is a bit odd (Exception only takes str) but other than that no complaints.

[ahopkins]

Message taking bytes is a bit odd

Not sure when or why that was added, but I think its been there a while.