Typing: replace "type" with specific metaclass #2181
Conversation
lafrech
force-pushed
the
typing_type
branch
from
60ec694
to
7385fd9
Compare
|
It looks like there are two last anonymous Changing the return type of The |
|
I applied @deckar01's commit and rebased. Guys, feel free to merge. |
src/marshmallow/schema.py
Outdated
| @@ -497,7 +498,7 @@ def _call_and_store(getter_func, data, *, field_name, error_store, index=None): | |||
| return error.valid_data or missing | |||
| return value | |||
|
|
|||
| def _serialize(self, obj: _T | typing.Iterable[_T], *, many: bool = False): | |||
| def _serialize(self, obj: dict | typing.Iterable[dict], *, many: bool = False): | |||
There was a problem hiding this comment.
this isn't quite right, is it? obj can be any arbitrary object type or an iterable objects.
There was a problem hiding this comment.
Per https://github.com/marshmallow-code/marshmallow/pull/2181/files#r1401344892, the issue was that _T was being used incorrectly in _deserialize. Moving it to load() would avoid the need to change this TypeVar.
There was a problem hiding this comment.
Per #2181 (comment) obj should be Any.
src/marshmallow/schema.py
Outdated
| @@ -510,7 +511,7 @@ def _serialize(self, obj: _T | typing.Iterable[_T], *, many: bool = False): | |||
| if many and obj is not None: | |||
| return [ | |||
| self._serialize(d, many=False) | |||
| for d in typing.cast(typing.Iterable[_T], obj) | |||
| for d in typing.cast(typing.Iterable[dict], obj) | |||
There was a problem hiding this comment.
related to comment above: i think this was already correct
There was a problem hiding this comment.
Per https://github.com/marshmallow-code/marshmallow/pull/2181/files#r1401346431, yes, this can be reverted.
There was a problem hiding this comment.
Per #2181 (comment) this should be Any and the cast is not necessary.
| @@ -584,7 +585,7 @@ def _deserialize( | |||
| partial=None, | |||
| unknown=RAISE, | |||
| index=None, | |||
| ) -> _T | list[_T]: | |||
| ) -> dict | list[dict]: | |||
There was a problem hiding this comment.
related to above: _deserialize is expected to return an arbitrary object or list of objects, not dictionaries.
There was a problem hiding this comment.
ret_d = self.dict_class() ... return ret_d implies that the return type of _deserialize() is the return type of dict_class() (Mapping). That change reveals that our usage of the return value actually depends on the dict interface. The fact that the type checker doesn't error on key access to a TypeVar which is effectively Any, seems like a mypy bug, but maybe it just needs explicit test coverage with a concrete type.
Enveloping occurs in _do_load(), so the return type of _deserialize() should not be constrained by the TypeVar. Now that I understand the purpose of the TypeVar it looks like it should be moved to load() rather than removed.
There was a problem hiding this comment.
When I tested my proposed change mypy errored with "A function returning TypeVar should receive at least one argument containing the same TypeVar". I thought _T was trying to enforce a generic type between two methods, but it can't actually do that. The type vars were only used to cast list items in recursive calls.
Since none of these methods actually pass through an input type, Any should be used and those recursive cast operations are unnecessary.
|
I tried to apply the changes discussed in comment. I'm not 100% sure I got everything right. mypy seems to be happy with those changes. |
See #2164.
Feedback welcome.