-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add types for Docker client from_env kwargs #11989
Conversation
stubs/docker/docker/client.pyi
Outdated
version: NotRequired[str] | ||
timeout: NotRequired[int] | ||
max_pool_size: NotRequired[int] | ||
environment: NotRequired[_Environ] |
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 chose to create / use the _Environ
protocol here.
The documentation says:
environment (dict): The environment to read environment variables
from. Default: the value of ``os.environ``
And the implementation includes:
if not environment:
environment = os.environ
I therefore considered dict[str, str]
, but I thought it best to allow users to pass in os.environ
.
This comment has been minimized.
This comment has been minimized.
stubs/docker/docker/client.pyi
Outdated
class DockerClient: | ||
api: APIClient | ||
def __init__(self, *args, **kwargs) -> None: ... | ||
@classmethod | ||
def from_env(cls, **kwargs) -> DockerClient: ... | ||
def from_env(cls, **kwargs: Unpack[_FromEnvDict]) -> DockerClient: ... |
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.
It's probably easier to just list the allowed arguments here, instead of using Unpack
. This is how we usually handle cases like this in typeshed.
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.
Thank you for the speedy response (as often!).
Would that mean, for example: def from_env(cls, version: str | None = None, ...)
?
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.
Exactly.
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.
Thank you. I made this change - however, unlike my sample above, I used a *
to mark the arguments as keyword-only. This was to avoid errors like error: docker.client.DockerClient.from_env is inconsistent, runtime does not have argument "use_ssh_client". Maybe you forgot to make it keyword-only in the stub?
in CI.
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.
Good point, using the star is correct, of course.
@@ -12,11 +13,25 @@ from docker.models.services import ServiceCollection | |||
from docker.models.swarm import Swarm | |||
from docker.models.volumes import VolumeCollection | |||
|
|||
@type_check_only | |||
class _Environ(Protocol): |
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 is copied from:
Lines 35 to 37 in a375953
class _Environ(Protocol): | |
def __getitem__(self, k: str, /) -> str: ... | |
def keys(self) -> Iterable[str]: ... |
Without this, we get errors like: ``` error: docker.client.DockerClient.from_env is inconsistent, runtime does not have argument "use_ssh_client". Maybe you forgot to make it keyword-only in the stub? ```
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Sebastian Rittau <srittau@rittau.biz>
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
No description provided.