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

Build: Initial mypy integration #1395

Merged
merged 9 commits into from
Jun 1, 2021

Conversation

jku
Copy link
Member

@jku jku commented May 17, 2021

Goal is to start using mypy to statically check our source code:

  • Add mypy configuration: start by just checking tuf/api, and ignoring securesystemslib imports
  • Use mypy in tox "lint" environment.
  • Refactor tox lint env a little
  • Use all cores for pylint to make it faster (pylint is the slowest part of the lint by far)
  • Fix all kinds of typing issues in tuf/api/ (either actual issues or just things that prevent mypy from doing its job)

How mypy is used after this PR:

  • CI will run it as part of lint
  • developers can run tox -e lint or just mypy

Fixes #1335

@jku
Copy link
Member Author

jku commented May 18, 2021

rebased on develop

Jussi Kukkonen added 9 commits May 19, 2021 14:26
The import is useful for mypy so it can check the types.
Add a pylint disable just like json.py does in the same situation.

Signed-off-by: Jussi Kukkonen <[email protected]>
Use ClassVar for extra protection, set default value to a string so type
checking is ok with it

Signed-off-by: Jussi Kukkonen <[email protected]>
Also define from_dict()/to_dict() as abstract: this helps mypy keep
track of things. Rename derived argument *_dict in the derived classes
to keep the linter happy.

Signed-off-by: Jussi Kukkonen <[email protected]>
This allows mypy to track the argument types through the constructor
calls.

Signed-off-by: Jussi Kukkonen <[email protected]>
This is an initial setup: By default check only tuf/api/,
and ignore securesystemslib imports.

Change lint working directory to source root: This saves repeating a lot
of {toxinidir} in the command lines.

Signed-off-by: Jussi Kukkonen <[email protected]>
pylint on the legacy code is by far the slowest part of linting (to
the extent that parallelizing the tox env itself doesn't really help):
pylint can fortunately parallelize itself.

Signed-off-by: Jussi Kukkonen <[email protected]>
Also mark the argument as Dict as we will pop() it.

Signed-off-by: Jussi Kukkonen <[email protected]>
Without this mypy figures the dict is Dict[str, str] and then promptly
fails when int value is inserted

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku force-pushed the initial-mypy-integration branch from 116047a to b643e5b Compare May 19, 2021 11:51
@jku
Copy link
Member Author

jku commented May 19, 2021

rebased on develop, added fix for an issue in new code

Copy link
Contributor

@sechkova sechkova left a comment

Choose a reason for hiding this comment

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

Do you think stricter options should be tried next? For example "disallow-untyped-defs" on api/metadata.py.
I see that now this results to errors coming from formats.py and exceptions.py.

Or maybe this is too strict and we can add type annotations to the coding guidelines for now?


if TYPE_CHECKING:
# pylint: disable=cyclic-import
from tuf.api.metadata import Metadata, Signed
Copy link
Contributor

Choose a reason for hiding this comment

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

So mypy recognizes that the quoted strings "Metadata" and "Signed" are the classes in this import?
Cool trick!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, AFAIK for mypy the type-names-as-string are just as valid.

@jku
Copy link
Member Author

jku commented May 24, 2021

Do you think stricter options should be tried next? For example "disallow-untyped-defs" on api/metadata.py.
I see that now this results to errors coming from formats.py and exceptions.py.

We probably should start experimenting with stricter options: once it's all set up I imagine it's not hard to maintain... But probably the most valuable improvement would be to gradually start supporting securesystemslib modules.

On the specific cases you mentioned: formats.py should go IMO: #1384. Fixing exceptions.py should be a few minutes job.

Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. Might take a while getting used to inline type annotations, but should be worth it 🤞

Will be great to include ngclient in the lint test Tox environment, once it has merged.

@jku jku merged commit d9a928e into theupdateframework:develop Jun 1, 2021
@sechkova sechkova mentioned this pull request Jul 9, 2021
3 tasks
@jku jku deleted the initial-mypy-integration branch December 30, 2024 09:09
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.

lint/CI: Use mypy for static type checking
3 participants