gabrieldemarmiesse / python-on-whales

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

Incorrect type of `ValidPortMapping` #601

Closed Tersaramor closed 3 weeks ago

Tersaramor commented 2 months ago

Incorrect type of ValidPortMapping

1) Type python_on_whales/utils.py:228

ValidPortMapping = Union[
    Tuple[Union[str, int], Union[str, int]],
    Tuple[Union[str, int], Union[str, int], str],
]

2) Use Case

def format_port_arg(port_object: ValidPortMapping) -> str:
    if len(port_object) == 1:
        return port_object[0]
    elif len(port_object) == 2:
        return f"{port_object[0]}:{port_object[1]}"
    elif len(port_object) == 3:
        return f"{port_object[0]}:{port_object[1]}/{port_object[2]}"
    else:
        raise ValueError(
            "The size of the tuples in the publish list must be 1, 2, or 3"
        )

3) Actual Result:

Mypy warns

error: Statement is unreachable  [unreachable]
                return port_object[0]

Also docs allow publish to have 1,2 or 3 elements for ValidPortMapping

Ports to publish, same as the `-p` argument in the Docker CLI.
Example are `[(8000, 7000) , ("127.0.0.1:3000", 2000)]` or `[("127.0.0.1:3000", 2000, "udp")]`. 
You can also use a single entry in the tuple to signify that you want a random free port on the host. For example:
 `publish=[(80,)]`.

4) Expected Result: Type is correct

ValidPortMapping = Union[
     Tuple[Union[str, int]],
    Tuple[Union[str, int], Union[str, int]],
    Tuple[Union[str, int], Union[str, int], str],
]
LewisGaul commented 2 months ago

Yes that seems right, the simple fix to ValidPortMapping would be a welcome PR.