✨ Added argument client_type to add custom logic later
#524
Conversation
|
I'm fine with this, although I think it might be preferable if an approach using
Yeah this is a reasonable concern, although |
| @@ -65,6 +65,7 @@ class ClientConfig: | |||
| compose_project_directory: Optional[ValidPath] = None | |||
| compose_compatibility: Optional[bool] = None | |||
| client_call: List[str] = field(default_factory=lambda: ["docker"]) | |||
| client_type: Literal["docker", "podman", "nerdctl", "unknown"] = "unknown" | |||
There was a problem hiding this comment.
Maybe should use an enum to make it clearer what's supported?
There was a problem hiding this comment.
I know enum is better supported than Literal in static type checkers, but it's mostly the fault of type checkers. I expect support will get better in the future. Using strings is more beginner-friendly and works the same with a dynamic type checker like beartype.
There was a problem hiding this comment.
Using enums is more friendly at least to certain users who prefer to use types as guidance for what values are allowed rather than docs/IDE support, but I'm fine with either way.
There was a problem hiding this comment.
The codebase uses string everywhere for this use case, there are already litterals here and there. So let's use Literal to be consistent with the rest of the codebase.
|
About your comment with "--version", I think we can always add this logic later if we want to. Parsing a output well is hard and not fool proof as the output might change with upgrade. We can always do a call to |
| @@ -102,6 +109,7 @@ def __init__( | |||
| compose_compatibility: Optional[bool] = None, | |||
| client_binary: str = "docker", | |||
| client_call: List[str] = ["docker"], | |||
| client_type: Literal["docker", "podman", "nerdctl", "unknown"] = "unknown", | |||
There was a problem hiding this comment.
Is this intended as a public attribute of the class? (it would be helpful if it was). If so does it need adding to docs somewhere?
There was a problem hiding this comment.
It could be in the future. For the moment, it's not necessary for the goal of this PR.
| @@ -21,7 +21,7 @@ | |||
| from .exceptions import DockerException | |||
|
|
|||
| # alias | |||
| docker = DockerClient() | |||
| docker = DockerClient(client_type="docker") | |||
There was a problem hiding this comment.
If this is because client_call defaults to ["docker"] then why not have this default be set in DockerClient__init__()? Your point about the ctr exe possibly being a symlink to something else still stands such that this may be incorrect (unless it actually gets to the point where POW downloads the client).
There was a problem hiding this comment.
That's true, but then the "unknown" option would never be used, and it has a purpose. It means "as a user I didn't think about it". When people do from python_on_whales import docker, we can reasonably assume that they really mean docker, even if you're right, a symlink would be annying here.
I try to follow the motto "easy things should be easy, hard things should be possible". If there is no "unknown", then it's impossible for the library to tell you that it's assuming incorrectly the type of client.
To be clear, I don't think users are reading the whole documentation when using POW, so I have to handle the case of backward compatibility + the fact that some users will use DockerClient without reading the description of every argument.
There was a problem hiding this comment.
Yeah, that's fine, using python_on_whales.docker just comes with the caveat of assuming if you have docker on your system it actually is a docker client, I don't have an issue with that.
|
Can we merge as-is? We can always improve later on if we want, by making the attribute public or by doing the --version thing. |
@LewisGaul that should help to do the xfail for the PR #523
The idea is that it's not required to specify it by a user, but if python-on-whales needs it at some point, we just ask the user to add it to the DockerCLient constructor.
We could call "something version" instead and try to parse the output, but this option is more explicit and faster because it doesn't require a call at every program start.