Skip to content

Conversation

lev-blit
Copy link
Contributor

The code that was causing issues before this was in create:

import docker

client = docker.DockerClient(base_url="my_docker_daemon_url")
image = client.images.build(...)[0]
container = client.containers.create(image)  # error: Missing named argument "detach" for "create" of "ContainerCollection"

added the fixes to run and parse_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

@lev-blit lev-blit changed the title fix unrequired literals to have a default value fix unrequired kwargs to have a default value Aug 30, 2025

This comment has been minimized.

@AlexWaygood AlexWaygood changed the title fix unrequired kwargs to have a default value docker: fix unrequired kwargs to have a default value Aug 30, 2025
@@ -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,
Copy link
Member

@brianschubert brianschubert Aug 31, 2025

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,
Copy link
Member

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:

Suggested change
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.

Comment on lines 52 to 55
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
Copy link
Member

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

@lev-blit lev-blit force-pushed the bugfix/docker-required-literals branch from 1d03727 to fa849da Compare September 6, 2025 12:53
@lev-blit
Copy link
Contributor Author

lev-blit commented Sep 6, 2025

Thanks for the review @brianschubert! :)

Think this is more correct, let me know if there is anything else to fix

Copy link
Member

@brianschubert brianschubert left a comment

Choose a reason for hiding this comment

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

Thanks! For future reference: please try to avoid force pushing once a PR's been reviewed, since that makes it harder for reviewers so see what changed. We always squash merge anyway

detach: Literal[False] = False,
detach: Literal[True] = True,
Copy link
Member

Choose a reason for hiding this comment

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

The two run overloads still overlap (only one should have a default value for detach), and I'm not sure why they've been reversed. I think these were working as expected and you can revert the run changes, or is there an issue I'm not seeing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the default seems to be False, that's why I reversed them (to have the common one be the bottom one)

regarding the overlap - if I change the top one from Literal[True] = True to Literal[True] then mypy (the one configured for my vscode setup) says that "Overloaded function signatures 1 and 2 overlap with incompatible return types"...

Copy link
Member

Choose a reason for hiding this comment

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

These still overlap when using Literal[True] = True (I'm not sure why mypy doesn't emit an error in that case. That might be a bug or the result of a tradeoff that permits some kinds of useful overlaps. See python/mypy#19803)

I'm unclear on what problem is being solved here. Could you clarify what the issue with the current run signature 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.

honestly, I'm not sure this needs changing... lol

I don't need it for sure, will remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to make sure I do the right thing - I want to add an overload for create which has detach: Literal[True] and returns Container, and the default detach: bool = False will return bytes?

Copy link
Member

Choose a reason for hiding this comment

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

Does create need overloads? The docs seem to indicate that it returns Container unconditionally: https://github.com/docker/docker-py/blob/6e6a273573fe77f00776b30de0685162a102e43f/docker/models/containers.py#L921-L922

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are... correct :) thank you!

This comment has been minimized.

@lev-blit
Copy link
Contributor Author

lev-blit commented Sep 6, 2025

The force push was "update branch with rebase" from the github UI... :)

Copy link
Member

@brianschubert brianschubert left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

github-actions bot commented Sep 6, 2025

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:508: error: Overlap between argument names and ** TypedDict items: "description", "name"  [misc]
+ discord/ext/commands/hybrid.py:508: error: Overlap between argument names and ** TypedDict items: "name", "description"  [misc]
- discord/ext/commands/hybrid.py:629: error: Overlap between argument names and ** TypedDict items: "description", "name"  [misc]
+ discord/ext/commands/hybrid.py:629: error: Overlap between argument names and ** TypedDict items: "name", "description"  [misc]

@brianschubert brianschubert merged commit 3e75cd8 into python:main Sep 6, 2025
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants