mypy: introduce type checking#1254
Conversation
All currently failing modules are excluded from reporting errors using follow-imports=silent and an exclusion list. Future work can whittle down this failing list. This change will start enforcing new modules and those currently passing. Includes some minor alphabetical reordering in tox.ini. Signed-off-by: Chris Patterson <cpatterson@microsoft.com>
cjp256
force-pushed
the
mypy-type-checking
branch
from
07683b7 to
f9ca401
Compare
holmanb
left a comment
holmanb
left a comment
There was a problem hiding this comment.
I think I'm on board with this.
This takes a non-invasive approach and encourages stronger typing going forward. It also adds a laundry list of "files to improve" directly to pyproject.toml, which may even serve as an nice target for potential Hactoberfest contributions or new contributors.
| commands = {envpython} -m flake8 {posargs:cloudinit/ tests/ tools/ setup.py} | ||
| deps = flake8 | ||
|
|
||
| [testenv:tip-mypy] |
There was a problem hiding this comment.
there's a testenv:mpypy with pinned dependency versions, but the tip- target has unpinned dependencies to get the latest deps. There are a couple of other tip- targets there so I just followed suit. At least that's what I assumed they're there for, whether useful or not, idk :D
There was a problem hiding this comment.
There are a couple of other tip- targets there so I just followed suit. At least that's what I assumed they're there for, whether useful or not, idk :D
I should have looked before asking, I didn't realize those were there. That makes sense to me.
| '^cloudinit/apport\.py$', | ||
| '^cloudinit/cmd/query\.py$', | ||
| '^cloudinit/config/cc_apk_configure\.py$', |
There was a problem hiding this comment.
I like the direction and intent. Given the size of the excludes list, I'd want to tackle a followup on this in one big PR after we merge this to avoid thrashing downstreams with multiple format-based paper cut PRs that add up to an annoyance over a period of time. Given that we have already done a fairly big non-functional reformat due to unittest restructuring, black and isort formatting. I wonder if it makes sense for us to chunk this work out and try to squeeze it in sooner rather than later so downstreams who feel that format-based impact for black/isort/unittests only have to cross this bridge with non-functional merge-conflcts in one set of PRs across the 21.4 to 22.1 barrier.
There was a problem hiding this comment.
This isn't a formatting concern though. This is actual refactoring to ensure types align correctly. Mypy doesn't complain about non-typed stuff. It complains when expected types don't line up, and we have thousands of them. I don't think this is a trivial "do it all in one or two commits" type exercise.
TheRealFalcon
left a comment
TheRealFalcon
left a comment
There was a problem hiding this comment.
Thanks @cjp256 ! This is something we've talked about a number of times, so it's nice to see this added in a way that's non-invasive but allows us to have a higher bar for typing in the future.
It sounds like everybody here is in agreement on this PR, even though there might be disagreement as to how to proceed moving forward, so I'm going to go ahead and merge this.
4 participants
All currently failing modules are excluded from reporting
errors using follow-imports=silent and an exclusion list.
Future work can whittle down this failing list. This change will
start enforcing new modules and those currently passing.
Includes some minor alphabetical reordering in tox.ini.
Signed-off-by: Chris Patterson cpatterson@microsoft.com