Draft
Conversation
spackbot-app
bot
added
commands
compilers
core
tgamblin
force-pushed
the
mypy-check-untyped-defs
branch
from
8798551 to
3d42421
Compare
1 task
tgamblin
added a commit
that referenced
this pull request
Feb 23, 2023
`colify` is an old module in Spack that still uses `**kwargs` liberally. We should be more explicit. Doing this eliminates the need for many checks (can't pass the wrong arg if it isn't allowed) and makes the function documentation more clear. I'm pulling this out of #35639 as I think it helps the code regardless. - [x] use named arguments instead of `kwargs`
tgamblin
added a commit
that referenced
this pull request
Feb 23, 2023
`colify` is an old module in Spack that still uses `**kwargs` liberally. We should be more explicit. Doing this eliminates the need for many checks (can't pass the wrong arg if it isn't allowed) and makes the function documentation more clear. I'm pulling this out of #35639 as I think it helps the code regardless. - [x] use named arguments instead of `kwargs`
tgamblin
force-pushed
the
mypy-check-untyped-defs
branch
from
3d42421 to
b1e6a24
Compare
`mypy` only understands `sys.platform == "win32"`, not indirect assignments of that value to things like `is_windows`. If we don't use the accepted platform checks, `mypy` registers many Windows-only symbols as not present on Linux, when it should skip the checks for platform-specific code.
tgamblin
force-pushed
the
mypy-check-untyped-defs
branch
from
b1e6a24 to
6b38f22
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mypycan do much more aggressive (and useful) type checking for us, but we can't use it because the Spack codebase doesn't yet pass the aggressive checking.The Spack codebase is getting pretty large and in some cases unwieldy, so static checking like this should help us. In particular, with
--check-untyped-defs, we could catch nasty and time-consuming issues like #35632, which cost several days of multiple peoples' time.Another big win for
mypychecks is that they can see through*imports like those that we use inpackage.pyfiles. So we could find undefined/unused variables in corepackage.py's, which we currently miss.At first glance, this is going to require over 1,000 fixes throughout the codebase, so we'll need to do it somewhat gradually. I started this branch to collect the needed changes as commits. We can keep it rebased on
develop, and we can pull commits off of here into other PRs to commit partial changes as they're ready.When this (finally) goes green, we can merge the
check_untyped_defs = trueline inpyproject.tomland start checking PRs more aggressively.If we get this in, we could consider even more aggressive checksum, like:
--disallow-untyped-calls--disallow-untyped-defs--disallow-incomplete-defsThose essentially mandate type annotations everywhere, so we may not want to go that far just yet.
check_untyped_defsinpyproject.tomlmypyhappy