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

Wrong and confusing type annotations #318

Open
XiaoHuiHui233 opened this issue Feb 23, 2023 · 1 comment
Open

Wrong and confusing type annotations #318

XiaoHuiHui233 opened this issue Feb 23, 2023 · 1 comment
Labels
bug Something isn't working

Comments

@XiaoHuiHui233
Copy link

XiaoHuiHui233 commented Feb 23, 2023

The wrong type annotation was used in the uniswap.uniswap.Uniswap class. The parameters provider, web3, factory_contract_addr and router_contract_addr of the constructor default to None but are not declared as Optional. This is very unfriendly to static type checkers (such as Pyright with strict type checking enabled). And it will generate a lot of type error prompts, but actually no problem.

class Uniswap:
    """
    Wrapper around Uniswap contracts.
    """
    ...

    def __init__(
        self,
        address: Union[AddressLike, str, None],
        private_key: Optional[str],
        provider: str = None,
        web3: Web3 = None,
        version: int = 1,
        default_slippage: float = 0.01,
        use_estimate_gas: bool = True,
        # use_eip1559: bool = True,
        factory_contract_addr: str = None,
        router_contract_addr: str = None,
        enable_caching: bool = False,
    ) -> None:
        ...

At the same time, the type annotation format of address and private_key is confusing. In Python3.9 and below, the recommended declaration method is Optional[T] to indicate that the parameter can be None. And in 3.10+, its recommended to use T | None(also Union[T, None]). But here we can see the two different formats of type annotations are being used at the same time, which is confusing.

I suggest changing it to the following:

class Uniswap:
    """
    Wrapper around Uniswap contracts.
    """
    ...

    def __init__(
        self,
        address: Optional[Union[AddressLike, str]],
        private_key: Optional[str],
        provider: Optional[str] = None,
        web3: Optional[Web3] = None,
        version: int = 1,
        default_slippage: float = 0.01,
        use_estimate_gas: bool = True,
        # use_eip1559: bool = True,
        factory_contract_addr: Optional[str] = None,
        router_contract_addr: Optional[str] = None,
        enable_caching: bool = False,
    ) -> None:
        ...

Also, I noticed that some function parameters/return values also have such annotations. Returns possible None but no Optional is declared. This will confuse the user for wasting more time debugging the type.

@XiaoHuiHui233 XiaoHuiHui233 added the bug Something isn't working label Feb 23, 2023
@ErikBjare
Copy link
Member

Thanks for notifying me.

These annotations used to be valid, as CI has been running passing typechecking for a while.

However, recently typecheckers like mypy (not only pylance) have started enforcing no-implicit-optional, which I remedied in this PR along with other typing issues reported with newer mypy: #320

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants