-
Notifications
You must be signed in to change notification settings - Fork 963
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
chore: add mypy #10874
Conversation
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>
c049ee4
to
3f1b9bb
Compare
@di I think this is good to go - I didn't add mypy to |
There was a problem hiding this 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.
There was a problem hiding this 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!
This reverts commit fffeadb.
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
I'll try to clarify as best as I understand!
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
If we wanted to allow this behavior, we would configure 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.
Yes! By running each time with CI, we lint everything in the We can opt in to stricter configurations one at a time, preventing a huge PR, and then finally flip the switch to be Here's a list of flags under
So I'd advise taking on each one, adding to the config, running the lint, fixing/confirming, and shipping changes in iterative fashion.
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 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>
Yes. Thanks for the clarification @miketheman! @di I'll let you push the big green button. |
* 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>
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