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

Add type hints and expose them via PEP561 #1112

Closed
pipermerriam opened this issue Oct 17, 2018 · 4 comments
Closed

Add type hints and expose them via PEP561 #1112

pipermerriam opened this issue Oct 17, 2018 · 4 comments

Comments

@pipermerriam
Copy link
Member

largely copy/pasted from ethereum/py-evm#1398

Background

Type hints allow us to perform static type checking, among other things. They raise the security bar by catching bugs at development time that, without type support, may turn into runtime bugs.

This stackoverflow answer does a great job at describing their main benefits.

What is wrong?

The web3.py library isn't written with type hints.

This needs to be fixed by:

  1. Adding all missing type hints.
  2. Enforcing type checking in CI

How

There does exist tooling (monkeytype) to the generation of type hints for existing code bases. From my personal experience monkeytype can be helpful but does still require manual fine tuning. Also, manually adding these type hints does serve as a great boost to the general understanding of the code base as it forces one to think about the code.

  1. Run mypy --follow-imports=silent --warn-unused-ignores --ignore-missing-imports --no-strict-optional --check-untyped-defs --disallow-incomplete-defs --disallow-untyped-defs --disallow-any-generics -p web3 -p ens

  2. Eliminate every reported error by adding the right type hint

  3. This should be done incrementally. The following steps are merely a suggestion on what those steps might be. A good target for pull request size is <400 lines of code changed but this isn't a hard rule.

  • individual web3._utils modules (3762 sloc)
  • web3.contract module (1561 sloc)
  • web3.middlewares module (1307 slot)
  • web3.providers module (1212 slot)
  • everything that's left.

In order to incrementally enforce these in CI the mypy command should be added to the lint section of the tox.ini and can be incrementally updated to only look at the modules which have had their type hints added.

  1. Send frequent pull requests for chunks of work

Definition of done

This issue is done when the following criteria are met:

  1. Type hinting is part of CI under the lint environment in the tox.ini file

It needs to be:

mypy --follow-imports=silent --warn-unused-ignores --ignore-missing-imports --no-strict-optional --check-untyped-defs --disallow-incomplete-defs --disallow-untyped-defs --disallow-any-generics -p eth
  1. Usage of type: ignore (silencing the type checker) is minimized and there's a reasonable explanation for its usage

  2. Expose the type hints in the same manner as was done in Enable discovery of type hints as per PEP561 eth-typing#10

Stretch goals

When this issue is done, stretch goals can be applied (and individually get funded) to tighten type support to qualify:

  1. `mypy --strict --follow-imports=silent --ignore-missing-imports --no-strict-optional -p web3 -p ens

  2. `mypy --strict --follow-imports=silent --ignore-missing-imports -p web3 -p ens

@johnsBeharry
Copy link

johnsBeharry commented Oct 18, 2018

hey @pipermerriam thanks for the clear requirements - I'm going to get start on this now.

ps. this is a fantastic initiative - as with anything we can to do minimise ambiguity.

johnsBeharry added a commit to peakshift/web3.py that referenced this issue Oct 18, 2018
@pipermerriam
Copy link
Member Author

@johnsBeharry 👍 please get a pull request opened for this as soon as you have anything committed (as that helps us give feedback early).

@johnsBeharry
Copy link

johnsBeharry commented Oct 19, 2018

Just an update - took a while for me to get all the tests running so I could trace with monkeytype I'm applying the suggestions selectively. Will open the PR with that so you can track progress @pipermerriam

@wolovim
Copy link
Member

wolovim commented Jun 1, 2020

Cleaning up stale issues; types have been implemented 🚢

@wolovim wolovim closed this as completed Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants