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. |
|
Thanks for the updates! The new config file syntax looks great to me. Left some more minor comments. After you've done everything else, it would be great it you can trim the test cases. |
| if not isinstance(value, str): | ||
| value = str(value) | ||
| if value.lower() not in configparser.RawConfigParser.BOOLEAN_STATES: | ||
| raise ValueError('Not a boolean: %s' % value) |
JukkaL
Apr 23, 2021
Collaborator
Similar to above, raise a custom error and make sure we handle it without generating a traceback.
Similar to above, raise a custom error and make sure we handle it without generating a traceback.
TheCleric
Apr 23, 2021
Author
Contributor
In this case, this is the same way that the RawConfigParser does it, so I'd recommend staying consistent with how it parses booleans vs. how we do so for the TOML file.
I.e., here's what's in configparser.py:
def _convert_to_boolean(self, value):
"""Return a boolean value translating from other types if necessary.
"""
if value.lower() not in self.BOOLEAN_STATES:
raise ValueError('Not a boolean: %s' % value)
return self.BOOLEAN_STATES[value.lower()]
In this case, this is the same way that the RawConfigParser does it, so I'd recommend staying consistent with how it parses booleans vs. how we do so for the TOML file.
I.e., here's what's in configparser.py:
def _convert_to_boolean(self, value):
"""Return a boolean value translating from other types if necessary.
"""
if value.lower() not in self.BOOLEAN_STATES:
raise ValueError('Not a boolean: %s' % value)
return self.BOOLEAN_STATES[value.lower()]|
@JukkaL I think other than the boolean func I left a comment on above, all the requested changes have been made. |
|
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 | ||
| -- ---------------------------------------- |
JukkaL
Apr 30, 2021
Collaborator
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.
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.
TheCleric
Apr 30, 2021
Author
Contributor
Sorry, got a little overzealous in the removal. Will do.
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) |
|
Thanks for the final batch of updates! Looks good now. |
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.