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

Improve mypy type checking #4553

Merged
merged 4 commits into from
Nov 12, 2020
Merged

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Nov 10, 2020

Currently, the mypy in the pre-commit is used as a "non-local" import and adds the blanket --ignore-missing-imports flag.
This greatly reduces the effectiveness of the type checking, because it does not check any types from classes/functions imported from third-party packages.
For example in #4534 I add the beautifully typed https://github.com/aiidateam/archive-path (note the py.typed file in the module signalling it is typed), but the code in aiida-core is not checked against these types at present.

Similarly, adding check_untyped_defs = True fixes this issue: https://mypy.readthedocs.io/en/stable/common_issues.html#no-errors-reported-for-obviously-wrong-code.

This PR is blocked until #4534 is merged (which should fix the current errors).

Note that if we did want to type check against e.g. django there is available stub packages: https://github.com/typeddjango/django-stubs

@chrisjsewell chrisjsewell added the pr/blocked PR is blocked by another PR that should be merged first label Nov 10, 2020
@codecov
Copy link

codecov bot commented Nov 10, 2020

Codecov Report

Merging #4553 (2457fe8) into develop (bd197f3) will decrease coverage by 0.01%.
The diff coverage is 62.50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4553      +/-   ##
===========================================
- Coverage    79.41%   79.41%   -0.00%     
===========================================
  Files          481      481              
  Lines        35279    35280       +1     
===========================================
  Hits         28013    28013              
- Misses        7266     7267       +1     
Flag Coverage Δ
django 73.57% <62.50%> (-<0.01%) ⬇️
sqlalchemy 72.74% <62.50%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/tools/importexport/archive/writers.py 92.04% <33.34%> (+0.04%) ⬆️
aiida/common/progress_reporter.py 92.46% <66.67%> (ø)
aiida/tools/importexport/dbexport/__init__.py 97.40% <100.00%> (ø)
aiida/orm/computers.py 68.23% <0.00%> (-0.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd197f3...2457fe8. Read the comment docs.

greschd
greschd previously approved these changes Nov 10, 2020
Copy link
Member

@greschd greschd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @chrisjsewell. I wasn't aware the pre-commit/mirrors-mypy adds a blanket --ignore-missing-imports, we definitely want to get rid of that.

This PR is blocked until #4534 is merged (which should fix the current errors).

I think it'd be ok to just ignore the migration errors until #4534 is merged, this doesn't necessarily have to block this PR.

@greschd
Copy link
Member

greschd commented Nov 10, 2020

By the way, should we also add a py.typed file, or does that only apply once a package is fully typed?

@chrisjsewell
Copy link
Member Author

By the way, should we also add a py.typed file, or does that only apply once a package is fully typed?

See https://www.python.org/dev/peps/pep-0561 for guidance. It seems maybe a bit premature to add one yet, given the sparsity of files checked just in .pre-commit-config.yaml (I'll pushing people to add more lol)

@chrisjsewell
Copy link
Member Author

this doesn't necessarily have to block this PR

well I'm hoping to merge very soon so might as well wait

@greschd
Copy link
Member

greschd commented Nov 10, 2020

See https://www.python.org/dev/peps/pep-0561 for guidance. It seems maybe a bit premature to add one yet, given the sparsity of files checked just in .pre-commit-config.yaml (I'll pushing people to add more lol)

There's this info:

If a stub package is partial it MUST include partial\n in a top level py.typed file.

But technically we're not a "stub package" I think - so does this still apply?

From this part, it seems to me the annotations should be at least somewhat complete when adding py.typed:

Package maintainers who wish to support type checking of their code MUST add a marker file named py.typed to their package supporting typing. This marker applies recursively: if a top-level package includes it, all its sub-packages MUST support type checking as well.

I guess that makes sense, because otherwise the calling code will get a bunch of errors if mypy can't automatically deduce the types.
It seems we could add py.typed only to a sub-package, but that would need checking.

@chrisjsewell chrisjsewell removed the pr/blocked PR is blocked by another PR that should be merged first label Nov 12, 2020
@chrisjsewell
Copy link
Member Author

ok this should be good to go now

@chrisjsewell
Copy link
Member Author

But technically we're not a "stub package" I think - so does this still apply?

no I don't believe this applies

It seems we could add py.typed only to a sub-package, but that would need checking.

yeh we can consider this later on, because there is not a whole lot of code that is type checked right now

greschd
greschd previously approved these changes Nov 12, 2020
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@greschd
Copy link
Member

greschd commented Nov 12, 2020

Good to go, thanks @chrisjsewell!

@chrisjsewell chrisjsewell changed the title 🧪 Improve mypy type checking Improve mypy type checking Nov 12, 2020
@chrisjsewell chrisjsewell merged commit 520bdbf into aiidateam:develop Nov 12, 2020
@chrisjsewell chrisjsewell deleted the improve-mypy branch November 12, 2020 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants