Add return type to Docker Container.logs - #11888
Conversation
The documentation at https://docker-py.readthedocs.io/en/stable/containers.html#docker.models.containers.Container.logs says "generator or str", but docker/docker-py#2240 will change this to say "generator of bytes or bytes".
This comment has been minimized.
This comment has been minimized.
| follow: Incomplete | None = None, | ||
| until: Incomplete | None = None, | ||
| ): ... | ||
| ) -> Generator[bytes, None, None] | bytes: ... |
There was a problem hiding this comment.
It seems to me that it would make sense to use an overload based on stream and demux:
- Return
CancellableStreamifstreamisTrue. - Return
Generator[tuple[bytes | None, bytes | None], None, None]ifdemuxisTrue. - Return
Generator[bytes, None, None]otherwise.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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: ...There was a problem hiding this comment.
Also, I just noticed: tail should be annotated as Literal["all"] | int, but that may be outside the scope of this PR.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@srittau I'm working on the assumption that where you put e.g. stream: True,, that should be stream: Literal[True],
This comment has been minimized.
This comment has been minimized.
| container, | ||
| stdout: bool = True, | ||
| stderr: bool = True, | ||
| *, |
There was a problem hiding this comment.
This * in particular I wasn't sure about. Maybe instead I should remove the = True on stdout and stderr?
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
The documentation at
https://docker-py.readthedocs.io/en/stable/containers.html#docker.models.containers.Container.logs says "generator or str", but docker/docker-py#2240 will change this to say "generator of bytes or bytes".