-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Allow per distribution mypy strictness options #5169
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
Conversation
# [mypy-tests] | ||
# [mypy-tests.yaml] | ||
# module_name = "yaml" | ||
# [mypy-tests.yaml.values] |
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 needs to have module_name = "yaml.values"
, right?
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 believe so. You have the mypy-tests.yaml section and underneath it the module_name value and the values section.
return | ||
|
||
assert isinstance(mypy_tests_conf, dict), "mypy-tests should be a section" | ||
for section_name, mypy_section in mypy_tests_conf.items(): |
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.
The section_name isn't actually used except in error messages. Could we use it instead of the module_name?
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.
The reason I put module_name in is because the module you use it for might not be a straight forward module or you might want to use a wildcard if you wanted the configuration to apply for several modules.
For example for protobuf, you can't put the section name as google.protobuf because the TOML heirarchy is dot based.
runs += 1 | ||
flags = [ | ||
"--python-version", "%d.%d" % (major, minor), | ||
"--config-file", config_file_name, | ||
"--strict-optional", | ||
"--no-site-packages", | ||
"--show-traceback", |
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.
We could instead put all of these options in the config file too, because (unless I'm misremembering) options passed at the command line trump those in a config file. But it's also useful to pass them here, so that we have a minimum set of strictness options that all packages have to obey.
My vote is to leave this as is for now, and revisit it if we see a need for less strict options in some particular package.
@@ -206,6 +262,8 @@ def main(): | |||
mypy_main("", sys.stdout, sys.stderr) | |||
except SystemExit as err: | |||
code = max(code, err.code) | |||
finally: |
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.
Why not put all the code in the with NamedTemporaryFile
block so you don't need to explicitly delete the file?
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.
Out of the tempfile python docs:
Whether the name can be used to open the file a second time, while the named temporary file is still open, varies across platforms (it can be so used on Unix; it cannot on Windows NT or later).
So the mypy process basically gets an empty file if you try to use it.
Good thing I develop on windows, or I'd never have guessed.
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 just stumbled onto https://bugs.python.org/issue14243 too, sounds painful.
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.
Thanks for the explanations!
As requested by #1526.
This addition takes mypy configuration from each distribution metadata file and constructs a single mypy.ini to run with. It assumes there is no mypy.ini but in case we ever need one, it would be simple to add these on top of an existing configuration file.
Might be relevant for #2852
As the issue did not really specify how the configuration would look, I added the following:
You may add a mypy-tests section to the metadata file.
It looks like this:
[mypy-tests]
[mypy-tests.yaml]
module_name = "yaml"
[mypy-tests.yaml.values]
disallow_incomplete_defs = true
disallow_untyped_defs = true
module_name can be of the form "a.*" like in mypy.ini.
You can add several module sections for complex distributions with several modules.
I added the '--warn-incomplete-stub' option since it is made specifically for typeshed runs. See docs.