feature: enable mypy to run without --ignore-imports #22844
Conversation
|
Pretty sure need to install test/functional/test_framework/test_framework.py:805: error: Cannot find implementation or library stub for module named 'zmq' [import]
test/functional/interface_zmq.py:32: error: Cannot find implementation or library stub for module named 'zmq' [import]
test/functional/interface_zmq.py:32: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
Found 2 errors in 2 files (checked 214 source files)
^---- failure generated from test/lint/lint-python.sh |
ah yes, good catch! |
mypy stubs were introduced in 21.0.1 install 22.2.1 (latest as of this commit)
ignore lief package remove --ignore-imports flag from mympy run make data/ importable
4926236 to
47995dd
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK |
|
Can you change the commit message for 065c64a to be something like: Can you also add a commit bumping mypy to the latest version. Looks like it's In either commit can you add / update the dependency here as well: https://github.com/bitcoin/bitcoin/tree/master/test#lint-tests. |
|
Concept ACK, less exceptions in mypy allows for more thorough checking. |
| fi | ||
|
|
||
| if ! mypy --ignore-missing-imports --show-error-codes $(git ls-files "test/functional/*.py" "contrib/devtools/*.py"); then | ||
| if ! mypy --show-error-codes $(git ls-files "test/functional/*.py" "contrib/devtools/*.py"); then |
There was a problem hiding this comment.
Something I've wondered about in the past is whether checking these in the same mypy invocation is a good idea. I think it will consider all the files part of the same top-level module, which might cause inconsistencies of different directories have same-named modules, for example.
That said, it works.
|
I've fixed this up / taken it over in #23212. |
a46f71b lint: enable mypy checking for missing imports (josibake) 22e6526 lint mypy 0.910 (fanquake) 6ae9c2e lint: install pyzmq (22.3.0) into linter environment (josibake) b93e229 doc: remove pointlessly duplicated linter version / install info (fanquake) Pull request description: This is bitcoin#22844 with issues addressed, and two additional commits. One to move to the latest mypy version in the CI, and another to remove what is just pointless duplication from the test README. There's no need to relist package versions and install instructions (meaning 2x the work when changing anything), when you can just link to `04_install.sh` which has the versions used and pip invocations to install them. ACKs for top commit: practicalswift: cr ACK a46f71b Tree-SHA512: 2900dea3901d03a846dc1ea912f217d152e803845516c7d941745ec1291d145590cd4bf2ddc497f7cf628119ba9905d7b1531836062aa85b384e39cf436f62c6
a46f71b lint: enable mypy checking for missing imports (josibake) 22e6526 lint mypy 0.910 (fanquake) 6ae9c2e lint: install pyzmq (22.3.0) into linter environment (josibake) b93e229 doc: remove pointlessly duplicated linter version / install info (fanquake) Pull request description: This is bitcoin#22844 with issues addressed, and two additional commits. One to move to the latest mypy version in the CI, and another to remove what is just pointless duplication from the test README. There's no need to relist package versions and install instructions (meaning 2x the work when changing anything), when you can just link to `04_install.sh` which has the versions used and pip invocations to install them. ACKs for top commit: practicalswift: cr ACK a46f71b Tree-SHA512: 2900dea3901d03a846dc1ea912f217d152e803845516c7d941745ec1291d145590cd4bf2ddc497f7cf628119ba9905d7b1531836062aa85b384e39cf436f62c6
This relates to #19389 by enabling mypy to run without
--ignore-imports. This is the minimum amount of work needed to get mypy running in the linter, after which annotations can be added to scripts incontrib/andtest_framework/to get the full benefit of running mypy in the linter.special notes
This also involved updating PyZMQ per #19389 (comment) .
how to test
after making these changes, I ran
./test/lint/lint-python.shand verified no errors.To be sure mypy would actually catch type errors when they existed, I added the following lines to
contrib/devtools/symbol-checker.py:First,
filenameneeded to be annotated as str, or else mypy treats it asAnyandAny +1is allowed. After annotatingfilename: strand adding an illegal operation, running ./test/lint/lint-python.sh` returned the following error:next steps
Where we do have mypy annotations, they are often incomplete. This causes mypy to evaluate arguments as
Any(see example above withsymbol-checker.py). I propose a follow-up PR adding annotations. At a minimum, annotations should be added for everything intest_framework/, which can be done fairly easily withmonkeytype. The benefit of this is, now that we are running without--ignore-imports, mypy can detect errors in the functional tests even if they don't have annotations, so long as all of the files intest_framework/are fully annotated.