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

chore: add mypy #10874

Merged
merged 17 commits into from
Mar 9, 2022
Merged

chore: add mypy #10874

merged 17 commits into from
Mar 9, 2022

Conversation

miketheman
Copy link
Member

@miketheman miketheman commented Mar 7, 2022

Starts #10865

See each commit for reasoning and descriptions.

Check out this branch, run pip install -r requirements/lint.txt && bin/lint for a nice success.

Signed-off-by: Mike Fiedler miketheman@gmail.com

@miketheman
Copy link
Member Author

I’ll correct the failing lint tomorrow. Feels weird to have to move lines to allow for an ignore, so I may also ignore the flake8 E501 on these two lines as well.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
These were suggested from running mypy on the codebase.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Set configuration for mypy.

Exclude some of the subdirectories we are not interested in testing to
speed up mypy execution.

Ignore any 3rd party modules that we do not have types for yet.
Added links that I could find to help track completion.

Does **not** set `strict` mode yet, since that's a bigger lift.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Eventually this command should fold into `bin/lint` and be removed.

For now, it's a convenient execution wrapper.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Callables are receiving dynamic attributes, something that isn't "usual".

See python/mypy#708

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
See python/mypy#4226

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Part of the SQLAlchemy extensions, which do not yet have reliable
stubs/plugins.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Should come along with sqlalchemy stubs.

See: https://docs.sqlalchemy.org/en/14/orm/extensions/mypy.html#using-declared-attr-and-declarative-mixins

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Surfaced via mypy, corrected!

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Surfaced via mypy, corrected.

Unclear why this wouldn't have been caught by other tools.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
mypy detected this as a type mismatch, as the internal variable name was
shadowing the externally-supplied one, and changing the type.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Adding a `# type: ignore` comment to a couple of places triggered
flake8's line too long check.

Running `make reformat` did nothing for these - black has outstanding
design issues with line length and comments.

See psf/black#1713 for one example.

Instead of changing the line structure to accommodate, ignore these two
cases, at least until the types can be fixed and the comments removed.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@miketheman
Copy link
Member Author

@di I think this is good to go - I didn't add mypy to bin/lint just yet, but could since it's passing right now.

Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

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

This LGTM, I'll let @ewdurbin review as well. Nice to see this already catching bugs.

bin/mypy Outdated Show resolved Hide resolved
@di di requested a review from ewdurbin March 8, 2022 23:46
Copy link
Member

@ewdurbin ewdurbin left a comment

Choose a reason for hiding this comment

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

I don't feel like I have enough understanding here to explicitly approve this 😅.

What configuration is allowing us to implement this gradually? Will mypy detect when files have changed, or does it just skip over unannotated code?

The only other concern I have here is the long list of explicitly untyped modules, is there some way for us to know when typing is added or stubs are available? Feels like it will fall on those motivated to keep track and send PRs which I suppose is fine.

I'm not at all against merging this and starting down the path, I'm interested to see what the impact on workflow will be for myself and other contributors!

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@miketheman
Copy link
Member Author

I'll try to clarify as best as I understand!

What configuration is allowing us to implement this gradually?

The config as I've started will warn if we added mypy configs that aren't used (useful to prevent config bloat), and examine every file in scope for potential inconsistencies, using type inference.

The fix made in 69a07b9 (#10874) is an example of this - the function signature declared that since: int and then redefined it inside the function scope to a datetime, so that triggered a mypy alert of:

warehouse/legacy/api/xmlrpc/views.py:550: error: Incompatible types in assignment (expression has type "datetime", variable has type "int")

If we wanted to allow this behavior, we would configure --allow-redefinition (ref) - but since there was a single instance in the entire codebase, it felt like a good opportunity to fix.

While this is not a bug per se, since the code works, and Python allows redeclaration of a variable, mypy points out that this is a confusing behavior to the reader of the code, so a rename to a more explicit variable helps comprehension and solves the error.

Will mypy detect when files have changed, or does it just skip over unannotated code?

Yes! By running each time with CI, we lint everything in the warehouse module. (We could also lint tests/ but that's likely overkill.)

We can opt in to stricter configurations one at a time, preventing a huge PR, and then finally flip the switch to be --strict and prevent things from entering the codebase at that time.

Here's a list of flags under --strict:

Strict mode; enables the following flags: --warn-unused-configs, --disallow-any-generics, --disallow-subclassing-any, --disallow-untyped-calls, --disallow-untyped-defs, --disallow-incomplete-defs, --check-untyped-defs, --disallow-untyped-decorators, --no-implicit-optional, --warn-redundant-casts, --warn-unused-ignores, --warn-return-any, --no-implicit-reexport, --strict-equality

So I'd advise taking on each one, adding to the config, running the lint, fixing/confirming, and shipping changes in iterative fashion.

The only other concern I have here is the long list of explicitly untyped modules, is there some way for us to know when typing is added or stubs are available? Feels like it will fall on those motivated to keep track and send PRs which I suppose is fine.

Not really, which is why I've added PR links that I could find for projects that may add typing in the future.

The typeshed project collects and maintains a lot of types-* packages, but it's really up to the package's to ship a py.typed file to signal they have type information, per PEP-561.

One excellent way to reduce the amount of untyped packages in scope is to... reduce the total number of packages in use. 😁 But barring that, it's reasonable to check back in on packages as part of their upgrade cycles.

Let me know if this makes sense!

Until itsdangerous 2.0 is included, this types package is needed.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@ewdurbin
Copy link
Member

ewdurbin commented Mar 9, 2022

Yes. Thanks for the clarification @miketheman! @di I'll let you push the big green button.

@di di merged commit 83208c5 into pypi:main Mar 9, 2022
@miketheman miketheman deleted the miketheman/mypy-10865 branch March 9, 2022 16:58
domdfcoding pushed a commit to domdfcoding/warehouse that referenced this pull request Jun 7, 2022
* chore(deps): install mypy in lint.in

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* chore(deps): include more types-* packages for mypy

These were suggested from running mypy on the codebase.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* chore: configure mypy

Set configuration for mypy.

Exclude some of the subdirectories we are not interested in testing to
speed up mypy execution.

Ignore any 3rd party modules that we do not have types for yet.
Added links that I could find to help track completion.

Does **not** set `strict` mode yet, since that's a bigger lift.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* chore: add mypy runner script

Eventually this command should fold into `bin/lint` and be removed.

For now, it's a convenient execution wrapper.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* lint: ignore dynamic properties

Callables are receiving dynamic attributes, something that isn't "usual".

See python/mypy#708

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* lint: ignore lambdas

See python/mypy#4226

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* lint: ignore hybrid_property repeat definitions

Part of the SQLAlchemy extensions, which do not yet have reliable
stubs/plugins.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* lint: ignore sqlalchemy declarative

Should come along with sqlalchemy stubs.

See: https://docs.sqlalchemy.org/en/14/orm/extensions/mypy.html#using-declared-attr-and-declarative-mixins

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* lint: a few more ignores

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* lint: interface methods shouldn't use self

Surfaced via mypy, corrected!

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* lint: correct subclass path

Surfaced via mypy, corrected.

Unclear why this wouldn't have been caught by other tools.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* lint: rename internal variable

mypy detected this as a type mismatch, as the internal variable name was
shadowing the externally-supplied one, and changing the type.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* lint: ignore flake8 line too long for ignored types

Adding a `# type: ignore` comment to a couple of places triggered
flake8's line too long check.

Running `make reformat` did nothing for these - black has outstanding
design issues with line length and comments.

See psf/black#1713 for one example.

Instead of changing the line structure to accommodate, ignore these two
cases, at least until the types can be fixed and the comments removed.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* Revert "chore: add mypy runner script"

This reverts commit fffeadb.

* test: include mypy in lint execution

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* chore(deps): include itsdangerous type stubs

Until itsdangerous 2.0 is included, this types package is needed.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
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.

3 participants