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.

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

Conversation

moosbruggerj
Copy link
Contributor

Fix __init__ signatures of ReturnDict and ReturnList

PR makes serializer argument in __init__ method of ReturnDict and ReturnList position only and otherwise includes list and dict type annotations from typeshed.

Related issues

Jakob Moosbrugger added 2 commits July 25, 2023 14:36
Copied and integrated init type stubs from typeshed for ReturnList and
ReturnDict.
_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.

@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.

Added source reference of copied __init__ definitions.
Added tests for copy and __reduce__ methods.
@moosbruggerj moosbruggerj requested review from intgr and sobolevn July 27, 2023 08:51
@intgr intgr changed the title Fix __init__ signatures of ReturnDict and ReturnList Fix ReturnDict and ReturnList __init__ signatures Aug 3, 2023
Copy link
Contributor

@intgr intgr left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

@intgr intgr merged commit 591e5bf into typeddjango:master Aug 3, 2023
@intgr
Copy link
Contributor

intgr commented Oct 4, 2023

Finally released in version 3.14.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

ReturnDict annotations incorrect
3 participants