search

gabrieldemarmiesse / python-on-whales

An awesome Python wrapper for an awesome Docker CLI!
MIT License
557 stars 102 forks source link

:sparkles: Added argument `client_type` to add custom logic later #524

Closed gabrieldemarmiesse closed 9 months ago

gabrieldemarmiesse commented 9 months ago

@LewisGaul that should help to do the xfail for the PR https://github.com/gabrieldemarmiesse/python-on-whales/pull/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.

LewisGaul commented 9 months ago

I'm fine with this, although I think it might be preferable if an approach using <ctr_exe> --version could be used.

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.

Yeah this is a reasonable concern, although client_type could be a property that only runs the command on demand, perhaps caching the result for use by the version property too. Also should be very fast to run <exe> --version!

gabrieldemarmiesse commented 9 months ago

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 --version if the client_type is unknown and we want to avoid the user having to type client_type=.... For me this PR and a call to --version are not incompatible.

gabrieldemarmiesse commented 9 months ago

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.