Add ContentType generic to Response
#2547
Conversation
| @@ -27,7 +27,7 @@ classifiers = [ | |||
| ] | |||
| dependencies = [ | |||
| "anyio>=3.4.0,<5", | |||
| "typing_extensions>=3.10.0; python_version < '3.10'", | |||
| "typing_extensions>=4.4.0; python_version < '3.13'", | |||
There was a problem hiding this comment.
default= was introduced in this version: https://typing-extensions.readthedocs.io/en/latest/#typing_extensions.TypeVar
Kludex
force-pushed
the
use-default-response-content
branch
from
afcbdb0
to
ba8a89b
Compare
| @@ -4,7 +4,7 @@ | |||
| # Testing | |||
| coverage==7.4.3 | |||
| importlib-metadata==7.0.1 | |||
| mypy==1.8.0 | |||
| mypy==1.9.0 | |||
There was a problem hiding this comment.
Last release supports default=.
| @@ -41,7 +49,7 @@ def __init__( | |||
| self.body = self.render(content) | |||
| self.init_headers(headers) | |||
|
|
|||
| def render(self, content: typing.Any) -> bytes: | |||
| def render(self, content: Content | None) -> bytes: | |||
There was a problem hiding this comment.
| was introduced in 3.10.
There was a problem hiding this comment.
We use __future__.annotations, and the pipeline passes. 👀
| Content = typing.Union[str, bytes] | ||
| SyncContentStream = typing.Iterable[Content] | ||
| AsyncContentStream = typing.AsyncIterable[Content] | ||
| _Content = typing.Union[str, bytes] |
There was a problem hiding this comment.
Just to confirm, this is a breaking change right?
There was a problem hiding this comment.
Oh yeah, it is...
You are right, but I'm not changing that in this PR. And I think there was an attempt to change the type, but it failed, and we had to revert. |
| Content = TypeVar("Content", default=typing.Any) | ||
|
|
||
|
|
||
| class Response(typing.Generic[Content]): |
There was a problem hiding this comment.
It seems to me like the reality is that the default should be str | bytes given what Response accepts. Then subclasses like StreamingResponse should override this generic with whatever they accept. If we make the default typing.Any then Response is not correctly typed. Since we're setting the default in this PR we might as well choose an appropriate value.
There was a problem hiding this comment.
I'm not changing this in this PR. There was a previous attempt to change the type to str | bytes | None, and it was reverted - I don't remember the reason.
There was a problem hiding this comment.
Ok. It would be good to find that reason.
There was a problem hiding this comment.
Ok I see. So yes the right thing to do would be to make a BaseResponse and make Response just one of the many concrete classes:
_ResponseDataT = TypeVar('_ResponseDataT')
class BaseResponse(Generic[_ResponseDataT]):
def render(self, data: _ResponseDataT | None) -> bytes:
raise NotImplementedError
class Response(BaseResponse[str | bytes]):
...
_JsonData = str | bytes | float | int | None | ...
class JsonResponse(BaseResponse[_JsonData]):
...There was a problem hiding this comment.
But the Response class does have the .render() method... So I don't see how I'd that? I can't make the classes stop inheriting Response, because that would be a breaking change.
There was a problem hiding this comment.
Yes, this is also what I think is unreasonable about Starlette. For example, StreamResponse and FileReponse do not use render, but they inherit this method.
There was a problem hiding this comment.
Are you suggesting adding a breaking change then?
There was a problem hiding this comment.
Can we do some naming to make it a non-breaking change or minimize the breakage? We could leave the render() method on the base class but mark it as deprecated vi a warning. Adding a BaseResponse class is not a breaking change. I think removing the render() method from eg StreamingResponse (because it inherits from BaseResponse and not Response) would also be okay.
At the time that PR was implemented the
default=parameter was not available onTypeVar, and mypy didn't support it.Now it's possible to achieve what that PR intended.