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.

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

Add return type to Docker Container.logs #11888

Merged
merged 5 commits into from
May 12, 2024
Merged

Add return type to Docker Container.logs #11888

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ef9f962
Add return type to Docker Container.logs
adamtheturtle May 10, 2024
96dbb34
Use 'CancellableStream' as a return type for docker logs call
adamtheturtle May 11, 2024
f4a54f6
Use @overload on Docker logs method to show different return types
adamtheturtle May 11, 2024
5207eb6
Merge remote-tracking branch 'origin/master' into types-docker-contai…
adamtheturtle May 12, 2024
8ccc147
Address review comments to handle overload correctly
adamtheturtle May 12, 2024
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
3 changes: 2 additions & 1 deletion stubs/docker/docker/api/container.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from _typeshed import Incomplete
from collections.abc import Generator

class ContainerApiMixin:
def attach(
Expand Down Expand Up @@ -77,7 +78,7 @@ class ContainerApiMixin:
since: Incomplete | None = None,
follow: Incomplete | None = None,
until: Incomplete | None = None,
): ...
) -> Generator[bytes, None, None] | bytes: ...
Copy link
Collaborator

@srittau srittau May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It seems to me that it would make sense to use an overload based on stream and demux:

  • Return CancellableStream if stream is True.
  • Return Generator[tuple[bytes | None, bytes | None], None, None] if demux is True.
  • Return Generator[bytes, None, None] otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I'll have a go at creating an overload - though this is something I have not done before.

I believe demux is not related to this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srittau - I have made an attempt to do this here (but not yet in the other file). I found it challenging to know what to do wrt e.g. an argument with a default followed by an argument without a default. I'd appreciate some tips / specific guidance on this change, and then I can apply it to the other file and hopefully to other changes I make to this repository.

Copy link
Collaborator

@srittau srittau May 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Arguments with defaults following arguments with defaults are a bit cumbersome, unfortunately. You need two overloads for that, one where all previous parameters are required, and one using the star import.

    @overload
    def logs(
        self,
        container,
        stdout: bool = True,
        stderr: bool = True,
        *,
        stream: True,
        timestamps: bool = False,
        tail: str = "all",
        since: Incomplete | None = None,
        follow: Incomplete | None = None,
        until: Incomplete | None = None,
    ) -> CancellableStream: ...
    @overload
    def logs(
        self,
        container,
        stdout: bool,
        stderr: bool,
        stream: True,
        timestamps: bool = False,
        tail: str = "all",
        since: Incomplete | None = None,
        follow: Incomplete | None = None,
        until: Incomplete | None = None,
    ) -> CancellableStream: ...
    @overload
    def logs(
        self,
        container,
        stdout: bool = True,
        stderr: bool = True,
        stream: False = False,
        timestamps: bool = False,
        tail: str = "all",
        since: Incomplete | None = None,
        follow: Incomplete | None = None,
        until: Incomplete | None = None,
    ) -> bytes: ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I just noticed: tail should be annotated as Literal["all"] | int, but that may be outside the scope of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I just noticed: tail should be annotated as Literal["all"] | int, but that may be outside the scope of this PR.

I've submitted that change in #11906 and I will block this PR on that one to avoid conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srittau I'm working on the assumption that where you put e.g. stream: True,, that should be stream: Literal[True],

def pause(self, container) -> None: ...
def port(self, container, private_port): ...
def put_archive(self, container, path, data): ...
Expand Down
3 changes: 2 additions & 1 deletion stubs/docker/docker/models/containers.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from _typeshed import Incomplete
from collections.abc import Generator
from typing import NamedTuple

from .resource import Collection, Model
Expand Down Expand Up @@ -39,7 +40,7 @@ class Container(Model):
def export(self, chunk_size=2097152): ...
def get_archive(self, path, chunk_size=2097152, encode_stream: bool = False): ...
def kill(self, signal: Incomplete | None = None): ...
def logs(self, **kwargs): ...
def logs(self, **kwargs) -> Generator[bytes, None, None] | bytes: ...
def pause(self): ...
def put_archive(self, path, data): ...
def remove(self, **kwargs) -> None: ...
Expand Down