gabrieldemarmiesse / python-on-whales

An awesome Python wrapper for an awesome Docker CLI!
MIT License
537 stars 100 forks source link

Avoid using `List` and `Dict` for function argument types #584

Open LewisGaul opened 3 months ago

LewisGaul commented 3 months ago

For example:

ValidImage = Union[str, Image]

class ImageCLI(DockerCLICaller):
    def remove(
        self,
        x: Union[ValidImage, List[ValidImage]],
        force: bool = False,
        prune: bool = True,
    ): ...

If you call this as so:

from python_on_whales import docker
my_tags = ["foo", "bar"]
docker.image.remove(my_tags)

Then running mypy gives:

tmp.py:4: error: Argument 1 to "remove" of "ImageCLI" has incompatible type "list[str]"; expected "str | Image | list[str | Image]"  [arg-type]
tmp.py:4: note: "List" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
tmp.py:4: note: Consider using "Sequence" instead, which is covariant

The reason that mypy is complaining is that the API accepts a List[str | Image], i.e. a list that can hold strings or images, but not a list of only strings. Lists are mutable, so with the function could modify or add elements - and (according to the type signature) it's allowed to add elements that are of type Image, which would violate the inferred type of my_tags.

One solution on the caller side is to annotate my_tags: List[str | Image], but this could require importing Image and generally just shouldn't be necessary. PoW isn't intending to mutate the list so this should be indicated using Sequence or Iterable or similar.

gabrieldemarmiesse commented 3 months ago

I don't mind typing with something more generic than a list. After all, we often just iterate on the arguments.

As long as it doesn't complexify the codebase too much, I'd be happy to accept a PR in this direction.

A small PR would be great so that we can have an example to illustrate the change and its consequences.

And thanks for the explanation, I'm glad to have learned something today :)

LewisGaul commented 3 months ago

It's not really a problem of being more generic, e.g. allowing sets/tuples (although that would be nice too). The problem is that if you annotate with List[str | Image] then the input must also be a List[str | Image] - passing a List[str] is invalid simply because the argument (a list) is a mutable type, even though PoW intends this to be valid since it won't mutate the argument.

Unfortunately if we switch to a more generic type we'll also have to change isinstance(x, list) checks...

gabrieldemarmiesse commented 3 months ago

I don't see many other solution than using a more generic type. And yes it will change the isinstance() calls but I believe that's a reasonnable amount of work and that the benefits are worth it. But of course, if you see another path to fix this issue, feel free to share