Skip to content
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.

Sign up for GitHub

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

Fix ReturnDict and ReturnList __init__ signatures #452

Merged
merged 3 commits into from
Aug 3, 2023
Merged

Fix ReturnDict and ReturnList __init__ signatures #452

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ca554e
Made serializer argument for ReturnList and ReturnDict kw only.
Jul 25, 2023
dfbd7e6
added tests for ReturnList and ReturnDict init signatures
Jul 25, 2023
515bd29
Make ReturnDict and ReturnList generic.
Jul 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 36 additions & 4 deletions rest_framework-stubs/utils/serializer_helpers.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,51 @@
from collections.abc import Iterator, MutableMapping
from typing import Any
from collections.abc import Iterable, Iterator, MutableMapping
from typing import Any, TypeVar, overload

from _typeshed import SupportsKeysAndGetItem
from rest_framework.exceptions import ErrorDetail
from rest_framework.fields import Field
from rest_framework.serializers import BaseSerializer

_T = TypeVar("_T")
_VT = TypeVar("_VT")
_KT = TypeVar("_KT")

class ReturnDict(dict):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make it generic, but I am unsure about the wider impact this change has.
From what i have seen so far, only the new tests would break with that change.

Would you make the serializer argument/instance variable generic as well? (with a lower bound of BaseSerializer[Any])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to make key/value generic, like dict, since it subclasses dict. Don't bother with generic serializer, unless there's a particular reason to do so.

serializer: BaseSerializer
def __init__(self, serializer: BaseSerializer = ..., *args, **kwargs): ...
# def __init__(self, serializer: BaseSerializer, *args, **kw @overload
intgr marked this conversation as resolved.
Show resolved Hide resolved
@overload
def __init__(self, *, serializer: BaseSerializer) -> None: ...
@overload
def __init__(self: dict[str, _VT], *, serializer: BaseSerializer, **kwargs: _VT) -> None: ...
@overload
def __init__(self, __map: SupportsKeysAndGetItem[_KT, _VT], *, serializer: BaseSerializer) -> None: ...
@overload
def __init__(
self: dict[str, _VT], __map: SupportsKeysAndGetItem[str, _VT], *, serializer: BaseSerializer, **kwargs: _VT
) -> None: ...
@overload
def __init__(self, __iterable: Iterable[tuple[_KT, _VT]], *, serializer: BaseSerializer) -> None: ...
@overload
def __init__(
self: dict[str, _VT], __iterable: Iterable[tuple[str, _VT]], *, serializer: BaseSerializer, **kwargs: _VT
) -> None: ...
# Next two overloads are for dict(string.split(sep) for string in iterable)
# Cannot be Iterable[Sequence[_T]] or otherwise dict(["foo", "bar", "baz"]) is not an error
@overload
def __init__(self: dict[str, str], __iterable: Iterable[list[str]], *, serializer: BaseSerializer) -> None: ...
@overload
def __init__(
self: dict[bytes, bytes], __iterable: Iterable[list[bytes]], *, serializer: BaseSerializer
) -> None: ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow these overloads are a lot more complicated than I imagined.

I'm not sure if ReturnDict makes sense with non-string keys. Only string keys are JSON-serializable. But I'm OK with keeping this as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I have seen so far, drf uses json.dumps or json.dump at some point, and the typeshed definition accepts Any as argument. At runtime, the key types are restricted to str, int, float, bool or None, from what I have seen in a brief investigation. I could limit the generics to those types, or even just to str, but I am unsure whether it is worth it to deviate from the typeshed definition, since they are complex.

def copy(self) -> ReturnDict: ...
def __reduce__(self) -> tuple[dict, tuple[dict]]: ...

class ReturnList(list):
serializer: BaseSerializer
def __init__(self, serializer: BaseSerializer = ..., *args, **kwargs): ...
@overload
def __init__(self, *, serializer: BaseSerializer) -> None: ...
@overload
def __init__(self, __iterable: Iterable[_T], *, serializer: BaseSerializer) -> None: ...
def __reduce__(self) -> tuple[dict, tuple[dict]]: ...

class BoundField:
Expand Down
128 changes: 128 additions & 0 deletions tests/typecheck/test_serializers.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,131 @@
@cached_property
def fields(self) -> BindingDict:
return super().fields

- case: test_return_list
main: |
from rest_framework import serializers
from rest_framework.utils.serializer_helpers import ReturnList

class TestSerializer(serializers.Serializer):
def test(self) -> None:
ReturnList([], serializer=self)
ReturnList((), serializer=self)
ReturnList(["1"], serializer=self)
ReturnList([{}], serializer=self)
ret = ReturnList(serializer=self)
reveal_type(ret) # N: Revealed type is "rest_framework.utils.serializer_helpers.ReturnList"

- case: test_return_list_serializer_argument_is_kw_only
parametrized:
- arg: ""
err: No overload variant of "ReturnList" matches argument type "TestSerializer"
- arg: "[],"
err: No overload variant of "ReturnList" matches argument types "List[<nothing>]", "TestSerializer"
main: |
from rest_framework import serializers
from rest_framework.utils.serializer_helpers import ReturnList

class TestSerializer(serializers.Serializer):
def test(self) -> None:
ReturnList({{ arg }} self)
out: |
main:6: error: {{ err }}
main:6: note: Possible overload variants:
main:6: note: def ReturnList(self, *, serializer: BaseSerializer[Any]) -> ReturnList
main:6: note: def [_T] ReturnList(self, Iterable[_T], /, *, serializer: BaseSerializer[Any]) -> ReturnList

- case: test_return_list_serializer_is_required
parametrized:
- arg: ""
err: All overload variants of "ReturnList" require at least one argument
- arg: "[]"
err: No overload variant of "ReturnList" matches argument type "List[<nothing>]"
main: |
from rest_framework import serializers
from rest_framework.utils.serializer_helpers import ReturnList

class TestSerializer(serializers.Serializer):
def test(self) -> None:
ReturnList({{ arg }})
out: |
main:6: error: {{ err }}
main:6: note: Possible overload variants:
main:6: note: def ReturnList(self, *, serializer: BaseSerializer[Any]) -> ReturnList
main:6: note: def [_T] ReturnList(self, Iterable[_T], /, *, serializer: BaseSerializer[Any]) -> ReturnList

- case: test_return_dict
main: |
from rest_framework import serializers
from rest_framework.utils.serializer_helpers import ReturnDict
from typing import Mapping

class TestSerializer(serializers.Serializer):
def test(self) -> None:
input: Mapping[str, str] = {}
ReturnDict({}, serializer=self)
ReturnDict([("a", "a")], serializer=self)
ReturnDict(input, serializer=self)
iterable = ""
sep = " "
ReturnDict((string.split(sep) for string in iterable), serializer=self)
ret = ReturnDict(serializer=self)
reveal_type(ret) # N: Revealed type is "rest_framework.utils.serializer_helpers.ReturnDict"

- case: test_return_dict_serializer_argument_is_kw_only
parametrized:
- arg: ""
err: No overload variant of "ReturnDict" matches argument type "TestSerializer"
- arg: "{},"
err: No overload variant of "ReturnDict" matches argument types "Dict[<nothing>, <nothing>]", "TestSerializer"
- arg: "[],"
err: No overload variant of "ReturnDict" matches argument types "List[<nothing>]", "TestSerializer"
- arg: "[('a', 'a')],"
err: No overload variant of "ReturnDict" matches argument types "List[Tuple[str, str]]", "TestSerializer"
main: |
from rest_framework import serializers
from rest_framework.utils.serializer_helpers import ReturnDict

class TestSerializer(serializers.Serializer):
def test(self) -> None:
ReturnDict({{ arg }} self)
out: |
main:6: error: {{ err }}
main:6: note: Possible overload variants:
main:6: note: def ReturnDict(self, *, serializer: BaseSerializer[Any]) -> ReturnDict
main:6: note: def ReturnDict(self, *, serializer: BaseSerializer[Any], **kwargs: Any) -> ReturnDict
main:6: note: def [_KT, _VT] ReturnDict(self, SupportsKeysAndGetItem[_KT, _VT], /, *, serializer: BaseSerializer[Any]) -> ReturnDict
main:6: note: def ReturnDict(self, SupportsKeysAndGetItem[str, Any], /, *, serializer: BaseSerializer[Any], **kwargs: Any) -> ReturnDict
main:6: note: def [_KT, _VT] ReturnDict(self, Iterable[Tuple[_KT, _VT]], /, *, serializer: BaseSerializer[Any]) -> ReturnDict
main:6: note: def ReturnDict(self, Iterable[Tuple[str, Any]], /, *, serializer: BaseSerializer[Any], **kwargs: Any) -> ReturnDict
main:6: note: def ReturnDict(self, Iterable[List[str]], /, *, serializer: BaseSerializer[Any]) -> ReturnDict
main:6: note: def ReturnDict(self, Iterable[List[bytes]], /, *, serializer: BaseSerializer[Any]) -> ReturnDict

- case: test_return_dict_serializer_is_required
parametrized:
- arg: ""
err: All overload variants of "ReturnDict" require at least one argument
- arg: "{}"
err: No overload variant of "ReturnDict" matches argument type "Dict[<nothing>, <nothing>]"
- arg: "[]"
err: No overload variant of "ReturnDict" matches argument type "List[<nothing>]"
- arg: "[('a', 'a')]"
err: No overload variant of "ReturnDict" matches argument type "List[Tuple[str, str]]"
main: |
from rest_framework import serializers
from rest_framework.utils.serializer_helpers import ReturnDict

class TestSerializer(serializers.Serializer):
def test(self) -> None:
ReturnDict({{ arg }})
out: |
main:6: error: {{ err }}
main:6: note: Possible overload variants:
main:6: note: def ReturnDict(self, *, serializer: BaseSerializer[Any]) -> ReturnDict
main:6: note: def ReturnDict(self, *, serializer: BaseSerializer[Any], **kwargs: Any) -> ReturnDict
main:6: note: def [_KT, _VT] ReturnDict(self, SupportsKeysAndGetItem[_KT, _VT], /, *, serializer: BaseSerializer[Any]) -> ReturnDict
main:6: note: def ReturnDict(self, SupportsKeysAndGetItem[str, Any], /, *, serializer: BaseSerializer[Any], **kwargs: Any) -> ReturnDict
main:6: note: def [_KT, _VT] ReturnDict(self, Iterable[Tuple[_KT, _VT]], /, *, serializer: BaseSerializer[Any]) -> ReturnDict
main:6: note: def ReturnDict(self, Iterable[Tuple[str, Any]], /, *, serializer: BaseSerializer[Any], **kwargs: Any) -> ReturnDict
main:6: note: def ReturnDict(self, Iterable[List[str]], /, *, serializer: BaseSerializer[Any]) -> ReturnDict
main:6: note: def ReturnDict(self, Iterable[List[bytes]], /, *, serializer: BaseSerializer[Any]) -> ReturnDict