-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add support for pulling configuration from pyproject.toml files #10219
Add support for pulling configuration from pyproject.toml files #10219
Conversation
Looking into the test failures. I didn't see these on my machine because I was using a |
…s for TOML parsing
Might need some help from someone with more Travis CI experience than I. It seems to be throwing strange python segmentation faults, which I am not sure where they are coming from. |
It looks like you may have encountered a mypyc bug (mypyc is a compiler we use to compile mypy to C extensions). I'll try to debug this, since debugging mypyc issues can be non-trivial. Sorry about that! |
Thanks so much! |
After fixing the above issue, compiled tests still fail for me. Here's what I see:
This actually looks like it could be a legitimate error that mypyc was able to catch through strict runtime type checking. The type for argument |
Thanks, I'll fix! |
@JukkaL any clue on the strange failures on 3.5.1 with mypyc? These seem to pass everywhere else (including locally on 3.5.1), but for some reason the output prints out of order on these. |
I think I'm narrowing down the issues with 3.5. TOML doesn't use an OrderedDict by default like config_parser does, so I'm working on overriding that. In Python 3.6+ dicts are ordered by default which is why this is the only version with an issue. Will have a fix soon. |
We might drop support for 3.5 soon (#9950), so maybe it's not worth worrying about? I'd also be OK with not supporting pyproject.toml on 3.5 while we wait to confirm that we can fully drop support for 3.5. |
I'd be fine either way. I'll defer to those who may know more than I. 😄 |
I'm fine with not supporting pyproject.toml on Python 3.5. In the next public release we can declare Python 3.5 as deprecated (even though Python 3.5 will likely still be mostly supported). Just give a clear error messages if somebody tries to use it on Python 3.5. |
It actually works fine in Python 3.5, it's just one of the tests is failing because the order we store the data in the dict is variable since they aren't ordered. But the actual functionality is fine. |
@JukkaL I think other than the boolean func I left a comment on above, all the requested changes have been made. |
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 updates! This is almost ready to merge. It would be good to have more tests (but not as many as there were originally). Using ValueError in the boolean value parser seems okay.
-- messages and 1 if there are. | ||
|
||
-- Directories/packages on the command line | ||
-- ---------------------------------------- |
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.
Could you add back some successful test cases as well that would cover all the essential features? It looks like all the test cases test failure modes.
Please add additional test cases to check-*.test
files, since they are much faster than command-line tests.
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.
Sorry, got a little overzealous in the removal. Will do.
Alright @JukkaL, I tried to find a happy medium between too few and too many tests. 😆 Let me know if I got a better balance this time. |
as black is against supporting tox.ini, this is extra important to me to eventually have all of our configurations in a single spot... Right now we can not configure both black and mypy into the same file as black lacks support for tox.ini @JelleZijlstra , watching is closely. (But am going to stand by the fact I think black should make the reverse effort here to support ini files as well, particularly if this does not gain traction again) |
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 final batch of updates! Looks good now.
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 this functionality! I'm looking forward to having just one config file in my project.
I didn't find any errors, but I think some code may be expressed a bit nicer.
Kind regards
# mypy global options: | ||
|
||
[tool.mypy] | ||
python_version = "2.7" |
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.
Is it wise to have deprecated python versions in new code?
@@ -34,6 +40,14 @@ def parse_version(v: str) -> Tuple[int, int]: | |||
return major, minor | |||
|
|||
|
|||
def try_split(v: Union[str, Sequence[str]], split_regex: str = '[,]') -> List[str]: |
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.
what is it called 'try_' for? This name implies the splitting may fail. Should it be just 'split'? Or 'polymorphic_split'?
|
||
# Reuse the ini_config_types and overwrite the diff | ||
toml_config_types = ini_config_types.copy() # type: Final[Dict[str, _INI_PARSER_CALLABLE]] | ||
toml_config_types.update({ |
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.
Would it be possible to use the |
operator?
tom_config_types = ini_config_types | {
'python_version': lambda s: parse_version(str(s)),
...
}
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.
Mypy works with 3.5+ and this change would break everything on <3.9
@@ -121,14 +162,27 @@ def parse_config_file(options: Options, set_strict_flags: Callable[[], None], | |||
else: | |||
config_files = tuple(map(os.path.expanduser, defaults.CONFIG_FILES)) | |||
|
|||
parser = configparser.RawConfigParser() | |||
config_parser = configparser.RawConfigParser() |
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 name change looks unnecessary and adds confusion: it differs only by whitespace.
if 'mypy' not in toml_data: | ||
continue | ||
toml_data = OrderedDict({'mypy': toml_data['mypy']}) | ||
parser = destructure_overrides(toml_data) # type: MutableMapping[str, Any] |
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 logic complicates the loop body; would it be hard to extract a method? I think the responsibilities are better separated with something like:
def create_parser(config_file):
if is_toml(config_file):
return toml_parser(config_file), toml_config_types
else:
return ini_parser(config_file), ini_config_types
...
try:
parser, config_types = create_parser(config_file)
except (toml.TomlDecodeError, configparser.Error, ConfigTOMLValueError) as err:
...
for k, v in updates.items(): | ||
setattr(options, k, v) | ||
options.report_dirs.update(report_dirs) | ||
|
||
for name, section in parser.items(): | ||
if name.startswith('mypy-'): | ||
prefix = '%s: [%s]: ' % (file_read, name) | ||
prefix = get_prefix(file_read, 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.
Actually, once we know we're parsing a TOML file, we know what get_prefix
is going to do. As such, this can also become part of the return values of the proposed create_parser
function.
return filemame.lower().endswith('.toml') | ||
|
||
|
||
def destructure_overrides(toml_data: "OrderedDict[str, Any]") -> "OrderedDict[str, Any]": |
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 had a hard time guessing what 'destructure' meant. Would 'overrides_by_module' be a better name? This reflects what is achieved, not what the code is doing.
if old_config_name not in result: | ||
result[old_config_name] = module_overrides | ||
else: | ||
for new_key, new_value in module_overrides.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.
Does allowing 'accidental but equivalent' double entries bring more convenience, or would they rather hide a dormant configuration error?
[style] Also, since this is the exceptional case, shouldn't it stand out as such?
if old_config_name in result:
raise ConfigTOMLValueError("...")
result[old_config_name] = module_overrides
if isinstance(value, bool): | ||
return value | ||
if not isinstance(value, str): | ||
value = str(value) |
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.
How is None
handled here?
if not is_toml(filename): | ||
return ", ".join("[mypy-%s]" % module for module in modules) | ||
|
||
return "module = ['%s']" % ("', '".join(sorted(modules))) |
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 looks a bit weird. The quotes can be applied directly:
quoted = '"{}"'.format
return "module = [%s]" % ", ".join(map(quoted, sorted(modules)))
Description
Closes #5205
This PR will add support to mypy for end users specifying configuration in a pyproject.toml file. . I also updated the documentation to indicate that this support has been added, along with some guidelines on adapting the ini configurations to the toml format.
Test Plan
I've added tests as best I could to mirror existing tests that use a mypy.ini file, but with a pyrproject.toml file, to ensure the configuration is being pulled correctly with the desired results.