Skip to content

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.

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

Merged
merged 3 commits into from
Sep 6, 2025
Merged

docker: fix unrequired kwargs to have a default value #14666

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9110878
fix unrequired literals to have a default value
lev-blit Aug 30, 2025
fa849da
fix wrong default types according to CR
lev-blit Sep 6, 2025
3239f38
only keep detach in create
lev-blit Sep 6, 2025
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
10 changes: 5 additions & 5 deletions stubs/docker/docker/models/containers.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ class ContainerCollection(Collection[Container]):
cpu_shares: int | None = None,
cpuset_cpus: str | None = None,
cpuset_mems: str | None = None,
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!

device_cgroup_rules: list[Incomplete] | None = None,
device_read_bps: list[Mapping[str, str | int]] | None = None,
device_read_iops: list[Mapping[str, str | int]] | None = None,
Expand Down Expand Up @@ -208,7 +208,7 @@ class ContainerCollection(Collection[Container]):
volumes: dict[str, dict[str, str]] | list[str] | None = None,
volumes_from: list[str] | None = None,
working_dir: str | None = None,
) -> bytes: ... # TODO: This should return a stream, if `stream` is True
) -> Container: ...
@overload
def run(
self,
Expand All @@ -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: bool = False,
device_cgroup_rules: list[Incomplete] | None = None,
device_read_bps: list[Mapping[str, str | int]] | None = None,
device_read_iops: list[Mapping[str, str | int]] | None = None,
Expand Down Expand Up @@ -303,7 +303,7 @@ class ContainerCollection(Collection[Container]):
volumes: dict[str, dict[str, str]] | list[str] | None = None,
volumes_from: list[str] | None = None,
working_dir: str | None = None,
) -> Container: ...
) -> bytes: ... # TODO: This should return a stream, if `stream` is True
def create( # type:ignore[override]
self,
image: str | Image,
Expand All @@ -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: bool = False,
device_cgroup_rules: list[Incomplete] | None = None,
device_read_bps: list[Mapping[str, str | int]] | None = None,
device_read_iops: list[Mapping[str, str | int]] | None = None,
Expand Down
Loading