-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
docker: fix unrequired kwargs to have a default value #14666
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
base: main
Are you sure you want to change the base?
Conversation
Diff from mypy_primer, showing the effect of this PR on open source code: prefect (https://github.com/PrefectHQ/prefect)
- src/prefect/cli/dev.py:319: error: Missing named argument "detach" for "create" of "ContainerCollection" [call-arg]
discord.py (https://github.com/Rapptz/discord.py)
- discord/ext/commands/hybrid.py:834: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/hybrid.py:834: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
- discord/ext/commands/hybrid.py:858: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/hybrid.py:858: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
- discord/ext/commands/hybrid.py:883: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/hybrid.py:883: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
- discord/ext/commands/hybrid.py:935: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/hybrid.py:935: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
- discord/ext/commands/bot.py:290: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/bot.py:290: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
- discord/ext/commands/bot.py:314: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/bot.py:314: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
|
@@ -234,7 +234,7 @@ class ContainerCollection(Collection[Container]): | |||
cpu_shares: int | None = None, | |||
cpuset_cpus: str | None = None, | |||
cpuset_mems: str | None = None, | |||
detach: Literal[True], | |||
detach: Literal[True] = True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite right -- this is part of a pair of overloads that changes the return type depending on if detach=True
is passed. Simplified:
@overload
def run(self, *, detach: Literal[False] = False) -> bytes:
@overload
def run(self, *, detach: Literal[True]) -> Container:
Supplying a default here would make the overloads overlap with incompatible return types. That's a problem, since type checkers and other tools won't know which return type to use when no argument for detach
is provided.
(Ordinarily pyright would catch this, but we have that check disabled in typeshed since there's some cases where overlapping overloads are necessary; this isn't one of them).
@@ -325,7 +325,7 @@ class ContainerCollection(Collection[Container]): | |||
cpu_shares: int | None = None, | |||
cpuset_cpus: str | None = None, | |||
cpuset_mems: str | None = None, | |||
detach: Literal[True], | |||
detach: Literal[True] = True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be:
detach: Literal[True] = True, | |
detach: bool = True, |
Unlike run
above, this method isn't overloaded over the type of detach
. I also don't see anything in the source that suggests passing detach=False
is invalid: https://github.com/docker/docker-py/blob/6e6a273573fe77f00776b30de0685162a102e43f/docker/models/containers.py#L918-L919
I suspect that both the type Literal[True]
and the missing default value are accidental artifacts from when the keyword arguments were copied over from one of run
's overloads in #12216.
def parse_host(addr: None, is_win32: Literal[True] = True, tls: bool = False) -> Literal["npipe:////./pipe/docker_engine"]: ... | ||
@overload | ||
def parse_host( | ||
addr: None, is_win32: Literal[False] = False, tls: bool = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as run
above; this would make the overloads overlap
The code that was causing issues before this was in
create
:added the fixes to
run
andparse_host
since I saw them already, checked against https://github.com/docker/docker-py/blob/main/docker/models/containers.py and https://github.com/docker/docker-py/blob/main/docker/utils/utils.py