Typings aren't generic enough when using response_class
#7109
Comments
|
If you could provide an example with the mypy errors, that would be great (or feel free to make a PR if you think you know how to fix it). |
|
I have tried to fix this, but got stuck on the fact that from __future__ import annotations
from typing import Generic, TypeVar
ClientResponseT = TypeVar('ClientResponseT', bound='ClientResponse')
class ClientSession(Generic[ClientResponseT]):
def __init__(self, response: type[ClientResponseT] | None = None) -> None:
if response is None:
self.response: type[ClientResponseT] = ClientResponse
else:
self.response = response
def request(self) -> ClientResponseT:
return self.response()
class ClientResponse:
...We can't really show that just |
|
It has a default, you can therefore type it with overloads. See for example this PR, which types a Generic based on the |
|
I don't really understand your comment and in the code there is an actual from __future__ import annotations
from typing import Generic, TypeVar
ClientResponseT = TypeVar("ClientResponseT", bound="ClientResponse")
class ClientSession(Generic[ClientResponseT]):
def __init__(self, response: type[ClientResponseT] | None = None) -> None:
if response is None:
self.response: type[ClientResponseT] = ClientResponse
else:
self.response = response
def request(self) -> ClientResponseT:
return self.response()
class ClientResponse:
...
class MyResponse(ClientResponse):
...
session = ClientSession(response=MyResponse)
reveal_type(session.request())
base_session = ClientSession()
reveal_type(base_session.request())The issue is with |
|
Yes, see the changes in that PR. You just need to add overloads. One would declare the type of The linked PR has 1 overload where the type is defined by |
|
The type ignore there is on the implementation, which won't be used by the type checker to infer the type, it will only use the overloads. (Your example is pretty much identical to the issue that PR is resolving). |
|
So, it becomes something like: |
from __future__ import annotations
from typing import Generic, TypeVar, overload
ClientResponseT = TypeVar("ClientResponseT", bound="ClientResponse")
class ClientSession(Generic[ClientResponseT]):
@overload
def __init__(self, response: type[ClientResponseT]) -> None:
...
@overload
def __init__(self: ClientSession[ClientResponse], response: None = None) -> None:
...
def __init__(self, response: type[ClientResponseT] | None = None) -> None:
if response is None:
self.response = ClientResponse
else:
self.response = response
def request(self) -> ClientResponseT:
return self.response()
class ClientResponse:
...
class MyResponse(ClientResponse):
...
session = ClientSession(response=MyResponse)
reveal_type(session.request())
base_session = ClientSession()
reveal_type(base_session.request())❯ mypy test.py
test.py:24: error: Incompatible return value type (got "ClientResponse", expected "ClientResponseT") [return-value]
test.py:36: note: Revealed type is "test.MyResponse"
test.py:39: note: Revealed type is "test.ClientResponse"
Found 1 error in 1 file (checked 1 source file)With: if response is None:
self.response: type[ClientResponseT] = ClientResponse
else:
self.response = response❯ mypy test.py
test.py:19: error: Incompatible types in assignment (expression has type "Type[ClientResponse]", variable has type "Type[ClientResponseT]") [assignment]
test.py:36: note: Revealed type is "test.MyResponse"
test.py:39: note: Revealed type is "test.ClientResponse"
Found 1 error in 1 file (checked 1 source file) |
|
Problem is that mypy type checks the implementation with the inner annotations, but really it should type check it once for each overload (python/mypy#8867). But, the second one seems correct, you just have to type ignore the assignment. The important thing is that the typing the end user sees will be correct and error-free. |
diff --git a/aiohttp/client.py b/aiohttp/client.py
index c4074577..16774ba8 100644
--- a/aiohttp/client.py
+++ b/aiohttp/client.py
@@ -28,6 +28,7 @@ from typing import (
Type,
TypeVar,
Union,
+ overload,
)
from multidict import CIMultiDict, MultiDict, MultiDictProxy, istr
@@ -162,11 +163,13 @@ class ClientTimeout:
# 5 Minute default read timeout
DEFAULT_TIMEOUT: Final[ClientTimeout] = ClientTimeout(total=5 * 60)
+
+_ClientResponseT = TypeVar("_ClientResponseT", bound="ClientResponse")
_RetType = TypeVar("_RetType")
@final
-class ClientSession:
+class ClientSession(Generic[_ClientResponseT]):
"""First-class interface for making HTTP requests."""
__slots__ = (
@@ -193,6 +196,64 @@ class ClientSession:
"_read_bufsize",
)
+ @overload
+ def __init__(
+ self,
+ base_url: Optional[StrOrURL] = None,
+ *,
+ connector: Optional[BaseConnector] = None,
+ cookies: Optional[LooseCookies] = None,
+ headers: Optional[LooseHeaders] = None,
+ skip_auto_headers: Optional[Iterable[str]] = None,
+ auth: Optional[BasicAuth] = None,
+ json_serialize: JSONEncoder = json.dumps,
+ request_class: Type[ClientRequest] = ClientRequest,
+ response_class: Type[_ClientResponseT],
+ ws_response_class: Type[ClientWebSocketResponse] = ClientWebSocketResponse,
+ version: HttpVersion = http.HttpVersion11,
+ cookie_jar: Optional[AbstractCookieJar] = None,
+ connector_owner: bool = True,
+ raise_for_status: Union[
+ bool, Callable[[_ClientResponseT], Awaitable[None]]
+ ] = False,
+ timeout: Union[_SENTINEL, ClientTimeout, None] = sentinel,
+ auto_decompress: bool = True,
+ trust_env: bool = False,
+ requote_redirect_url: bool = True,
+ trace_configs: Optional[List[TraceConfig]] = None,
+ read_bufsize: int = 2**16,
+ ) -> None:
+ ...
+
+ @overload
+ def __init__(
+ self: "ClientSession[ClientResponse]",
+ base_url: Optional[StrOrURL] = None,
+ *,
+ connector: Optional[BaseConnector] = None,
+ cookies: Optional[LooseCookies] = None,
+ headers: Optional[LooseHeaders] = None,
+ skip_auto_headers: Optional[Iterable[str]] = None,
+ auth: Optional[BasicAuth] = None,
+ json_serialize: JSONEncoder = json.dumps,
+ request_class: Type[ClientRequest] = ClientRequest,
+ response_class: Type[_ClientResponseT] = ClientResponse,
+ ws_response_class: Type[ClientWebSocketResponse] = ClientWebSocketResponse,
+ version: HttpVersion = http.HttpVersion11,
+ cookie_jar: Optional[AbstractCookieJar] = None,
+ connector_owner: bool = True,
+ raise_for_status: Union[
+ bool, Callable[[ClientResponse], Awaitable[None]]
+ ] = False,
+ timeout: Union[_SENTINEL, ClientTimeout, None] = sentinel,
+ auto_decompress: bool = True,
+ trust_env: bool = False,
+ requote_redirect_url: bool = True,
+ trace_configs: Optional[List[TraceConfig]] = None,
+ read_bufsize: int = 2**16,
+ ) -> None:
+ ...
+
def __init__(
self,
base_url: Optional[StrOrURL] = None,This causes these errors: aiohttp/client.py:240: error: Incompatible default for argument "response_class" (default has type "Type[ClientResponse]", argument has type "Type[_ClientResponseT]") [assignment]
aiohttp/client.py:257: error: Overloaded function implementation does not accept all possible arguments of signature 1 [misc]Besides, adding an overload for every combination of parameters doesn't seem maintainable in the long run.. |
Don't put concrete values for defaults in. Overloads should be stubs, all values should be
This may be a mistake somewhere, or the same cause with the defaults. If you want to start making a PR, we can look at any remaining errors in there.
It's 2 overloads to handle the ClientResponse type. If you want it to be generic, that's the only option I'm aware of. If we want to make it generic over more types, then it could become a bit of a problem, potentially requiring an exponential number of overloads. e.g. If we want to be generic over both request_class and response_class, then it probably needs 4 overloads. If a third thing is needed, it might become 8 overloads. To avoid that, we'd probably need to rearchitecture the class in some way, to separate out these generics. |
|
Yeah I wonder if the restructure isn't better anyway.. I haven't looked at However, I don't really have a good idea for a restructuring. It feels like we're more limited by the expressiveness of the typing system rather than actual poorly designed classes. |
|
One option could potentially be to have a We did something like this for aiomcache's Not sure if it's the best approach, just an idea I thought of. |
|
If you're happy to play with it a little more, I think it may be possible to do this without overloads by using Looks like it'll probably make it into Python 3.12: https://peps.python.org/pep-0696/#using-another-typevarlike-as-default |
|
That |
|
We might need to wait for this to become generally available. Upgrading Free type variable expected in Generic[...]diff --git a/aiohttp/client.py b/aiohttp/client.py
index c4074577..c55755ef 100644
--- a/aiohttp/client.py
+++ b/aiohttp/client.py
@@ -31,7 +31,7 @@ from typing import (
)
from multidict import CIMultiDict, MultiDict, MultiDictProxy, istr
-from typing_extensions import Final, final
+from typing_extensions import Final, TypeVar as TypeVarFuture, final
from yarl import URL
from . import hdrs, http, payload
@@ -163,10 +163,11 @@ class ClientTimeout:
DEFAULT_TIMEOUT: Final[ClientTimeout] = ClientTimeout(total=5 * 60)
_RetType = TypeVar("_RetType")
+_ClientResponseT = TypeVarFuture("_ClientResponseT")
@final
-class ClientSession:
+class ClientSession(Generic[_ClientResponseT]):
"""First-class interface for making HTTP requests."""
__slots__ = (
diff --git a/requirements/constraints.txt b/requirements/constraints.txt
index cbb29329..912f7e5a 100644
--- a/requirements/constraints.txt
+++ b/requirements/constraints.txt
@@ -107,7 +107,7 @@ multidict==5.2.0
# via
# -r requirements/multidict.txt
# yarl
-mypy==0.982 ; implementation_name == "cpython"
+mypy==0.991 ; implementation_name == "cpython"
# via
# -r requirements/lint.txt
# -r requirements/test.txt
@@ -237,7 +237,7 @@ trustme==0.9.0 ; platform_machine != "i686"
# via -r requirements/test.txt
typer==0.4.0
# via python-on-whales
-typing-extensions==4.1.1
+typing-extensions==4.4.0
# via
# -r requirements/typing-extensions.txt
# aioredis
diff --git a/requirements/lint.txt b/requirements/lint.txt
index 1bab0a04..1f6c446a 100644
--- a/requirements/lint.txt
+++ b/requirements/lint.txt
@@ -1,6 +1,6 @@
-r typing-extensions.txt
aioredis==2.0.1
-mypy==0.982; implementation_name=="cpython"
+mypy==0.991; implementation_name=="cpython"
pre-commit==2.17.0
pytest==6.2.5
slotscheck==0.8.0
diff --git a/requirements/test.txt b/requirements/test.txt
index 1ab1c614..fdc43956 100644
--- a/requirements/test.txt
+++ b/requirements/test.txt
@@ -3,7 +3,7 @@ Brotli==1.0.9
coverage==6.4.2
cryptography==36.0.1; platform_machine!="i686" # no 32-bit wheels; no python 3.9 wheels yet
freezegun==1.1.0
-mypy==0.982; implementation_name=="cpython"
+mypy==0.991; implementation_name=="cpython"
mypy-extensions==0.4.3; implementation_name=="cpython"
proxy.py ~= 2.4.4rc3
pytest==6.2.5
diff --git a/requirements/typing-extensions.txt b/requirements/typing-extensions.txt
index 37e36b21..7849d5da 100644
--- a/requirements/typing-extensions.txt
+++ b/requirements/typing-extensions.txt
@@ -1 +1 @@
-typing_extensions==4.1.1
+typing_extensions==4.4.0
|
|
OK, mypy's not ready yet. That bug should be fixed in the next release: python/mypy#14312 |
|
Feature has been merged: python/mypy#14872 |
Describe the bug
The
ClientSessionclass allows you to specify aresponse_classwhich is then used for responses in place ofClientResponsewhen handling responses. However the typings are not generic, they are hard coded to useClientResponseso custom aspects of theresponse_classresult in type errors.To Reproduce
response_classClientSession, e.g.client.post()response_classin 1 will be used, but it will still be typed as the defaultClientResponseExpected behavior
Use the
response_classtype in typings.Versions
Code of Conduct
The text was updated successfully, but these errors were encountered: