GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Wrong and confusing type annotations · Issue #318 · uniswap-python/uniswap-python

Wrong and confusing type annotations #318

New issue
Copy link
New issue
Copy link
Open
Open
Wrong and confusing type annotations#318
Copy link
Labels
bugSomething isn't workingSomething isn't working

Description

@XiaoHuiHui233
XiaoHuiHui233
opened on Feb 23, 2023
Issue body actions

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.

Activity

  1. XiaoHuiHui233
    added
    bugSomething isn't workingSomething isn't working
    on Feb 23, 2023
  2. ErikBjare commented on Feb 24, 2023

    @ErikBjare
    ErikBjare
    on Feb 24, 2023
    Member
    More actions

    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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions

      You are visitor 01402503 — GeezerVison does not store the pages you look up.
      Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.