Skip to content

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

Merged
merged 2 commits into from
Apr 8, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions tests/mypy_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import re
import sys
import toml
import tempfile
from typing import Dict, NamedTuple

PY2_NAMESPACE = "@python2"
THIRD_PARTY_NAMESPACE = "stubs"
Expand Down Expand Up @@ -118,6 +120,46 @@ def add_files(files, seen, root, name, args, exclude_list):
files.append(fn)


class MypyDistConf(NamedTuple):
module_name: str
values: Dict

# The configuration section in the metadata file looks like the following, with multiple module sections possible
# [mypy-tests]
# [mypy-tests.yaml]
# module_name = "yaml"
# [mypy-tests.yaml.values]
Copy link
Member

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?

Copy link
Contributor Author

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.

# disallow_incomplete_defs = true
# disallow_untyped_defs = true


def add_configuration(configurations, seen_dist_configs, distribution):
if distribution in seen_dist_configs:
return

with open(os.path.join(THIRD_PARTY_NAMESPACE, distribution, "METADATA.toml")) as f:
data = dict(toml.loads(f.read()))

mypy_tests_conf = data.get("mypy-tests")
if not mypy_tests_conf:
return

assert isinstance(mypy_tests_conf, dict), "mypy-tests should be a section"
for section_name, mypy_section in mypy_tests_conf.items():
Copy link
Member

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?

Copy link
Contributor Author

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.

assert isinstance(mypy_section, dict), "{} should be a section".format(section_name)
module_name = mypy_section.get("module_name")

assert module_name is not None, "{} should have a module_name key".format(section_name)
assert isinstance(module_name, str), "{} should be a key-value pair".format(section_name)

values = mypy_section.get("values")
assert values is not None, "{} should have a values section".format(section_name)
assert isinstance(values, dict), "values should be a section"

configurations.append(MypyDistConf(module_name, values.copy()))
seen_dist_configs.add(distribution)


def main():
args = parser.parse_args()

Expand All @@ -142,6 +184,8 @@ def main():
for major, minor in versions:
files = []
seen = {"__builtin__", "builtins", "typing"} # Always ignore these.
configurations = []
seen_dist_configs = set()

# First add standard library files.
if major == 2:
Expand Down Expand Up @@ -177,17 +221,29 @@ def main():
if mod in seen or mod.startswith("."):
continue
add_files(files, seen, root, name, args, exclude_list)
add_configuration(configurations, seen_dist_configs, distribution)

if files:
with tempfile.NamedTemporaryFile("w+", delete=False) as temp:
temp.write("[mypy]\n")

for dist_conf in configurations:
temp.write("[mypy-%s]\n" % dist_conf.module_name)
for k, v in dist_conf.values.items():
temp.write("{} = {}\n".format(k, v))

config_file_name = temp.name
runs += 1
flags = [
"--python-version", "%d.%d" % (major, minor),
"--config-file", config_file_name,
"--strict-optional",
"--no-site-packages",
"--show-traceback",
Copy link
Member

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.

"--no-implicit-optional",
"--disallow-any-generics",
"--disallow-subclassing-any",
"--warn-incomplete-stub",
# Setting custom typeshed dir prevents mypy from falling back to its bundled
# typeshed in case of stub deletions
"--custom-typeshed-dir", os.path.dirname(os.path.dirname(__file__)),
Expand All @@ -206,6 +262,8 @@ def main():
mypy_main("", sys.stdout, sys.stderr)
except SystemExit as err:
code = max(code, err.code)
finally:
Copy link
Member

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?

Copy link
Contributor Author

@hatal175 hatal175 Apr 8, 2021

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.

Copy link
Member

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.

os.remove(config_file_name)
if code:
print("--- exit status", code, "---")
sys.exit(code)
Expand Down