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.

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

Avoid using List and Dict for function argument types #584

Open
LewisGaul opened this issue May 1, 2024 · 3 comments
Open

Avoid using List and Dict for function argument types #584

LewisGaul opened this issue May 1, 2024 · 3 comments
Labels
good first issue Good for newcomers

Comments

@LewisGaul
Copy link
Collaborator

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
Copy link
Owner

gabrieldemarmiesse commented May 1, 2024

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
Copy link
Collaborator Author

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
Copy link
Owner

gabrieldemarmiesse commented May 2, 2024

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

@LewisGaul LewisGaul added the good first issue Good for newcomers label May 20, 2024
gabrieldemarmiesse pushed a commit that referenced this issue May 29, 2024
…#595)

First step in working on
#584.

- `Command.add_args_list()` previously had a misleading name and type
annotation - it is used for accepting either a list or a single item.
Renamed to `Command.add_args_iterable_or_single()`.
- Added `Command.add_args_iterable()` for the case you know you're
dealing with an iterable (more efficient than always creating a new
list).
- Updated `utils.to_list()` to support generic iterables (still always
returns a list, as per the name - my plan is to phase out its use where
it's not needed).
- Updated `utils.format_dict_for_cli()` to support generic mappings.
- Added `Command.add_args_mapping()` as a convenience that wraps
`utils.format_dict_for_cli()`, which will allow tidying up code that
constructs commands.

Next step is to tidy up the components' code that constructs commands,
allowing arguments to be `Iterable` and `Mapping` in place of `List` and
`Dict`. With these changes it should be fairly straightforward, simply
calling the correct method on the `Command` class.

Here's an example of what upcoming changes look like:
LewisGaul/python-on-whales@command-construction...command-construction-image
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants