Skip to content
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 to pyproject.toml #5208

Closed
wants to merge 3 commits into from

Conversation

pslacerda
Copy link

@pslacerda pslacerda commented Jun 12, 2018

Support for TOML configuration files with sections like [tool.mypy] and [tool.mypy-mypackage.*].

Related to #5205.

@pslacerda
Copy link
Author

All these config files are in fact very much the same, except for the .toml difference. Maybe they all could be combined into a single variable:

CONFIG_FILES = ('pyproject.toml', 'mypy.ini', 'setup.cfg', '~/.mypy.ini')

Sorry for the # type: ignore, I'm working on it.

Nested TOML tables/sections (like [tool][mypy-package][module] == [tool.mypy-package.module]) need to be flatten. https://github.com/python/mypy/pull/5208/files#diff-b5e3909c86f7834f70af20bb848acec3R910

Dict that were a SectionProxy that is a Dict again, that is somewhat dirty. Maybe change everything to .toml would be saner. https://github.com/python/mypy/pull/5208/files#diff-b5e3909c86f7834f70af20bb848acec3R1008

That library could be packed together. I suppose the authour would be very happy. Or another can also be made or found. https://github.com/python/mypy/pull/5208/files#diff-2eeaed663bd0d25b7e608891384b7298R107

To test I just changed some existing ones.

@pslacerda pslacerda changed the title Add support for configure with pyproject.toml Add support to configure mypy with pyproject.toml Jun 12, 2018
@ethanhs
Copy link
Collaborator

ethanhs commented Jun 12, 2018

It appears that your changes are causing the tests to hang. Also typing doesn't have Type in 3.5.1. It seems you aren't using it anyway?

@ethanhs
Copy link
Collaborator

ethanhs commented Jun 26, 2018

It seems the tests timed out or crashed. I'm going to close/reopen to try them one more time. If they still fail @pslacerda can you look into why they are failing?

@ethanhs ethanhs closed this Jun 26, 2018
@ethanhs ethanhs reopened this Jun 26, 2018
@pslacerda
Copy link
Author

I have no idea why it is timing out. Seems really unrelated to this modifications regards functionallity, but related to testing probably. I will look into this soon.

@gvanrossum
Copy link
Member

gvanrossum commented Aug 2, 2018

Honestly, we should never have caved in to supporting setup.cfg, then this whole thing would not have been necessary. It seems just busywork. Unlike the original motivation given in PEP 518 (which is to specify setup-time dependencies), there's nothing wrong with mypy.ini -- this PR simply does not solve a problem we have.

I did say that just submitting the PR wouldn't mean I'd accept it, and having thought it over more I am just going to reject this PR. Sorry!

@gvanrossum gvanrossum closed this Aug 2, 2018
@touilleMan
Copy link

Hi,

I'm not complaining about this PR not being merged, but it has raised a question about python project organization:

I would have considered good practice to keep all tool configuration into a single file (keep the root directory cleaner, allow to understand project tooling by just looking at a single place) be it setup.cfg in the past and now this fancy pyproject.toml ;-)
However you seem to consider supporting configuration from setup.cfg was also a mistake, then it would mean good practice for project would be to a configuration file for each tool and avoid setup.cfg totally and use pyproject.toml only for setup-time dependency resolution and project building ?

@gvanrossum
Copy link
Member

Have you seen this? https://xkcd.com/927/

@pslacerda
Copy link
Author

But sure, jokes aside, there is only setup.cfg and pyproject.toml competing, even if setup.cfg don't really compete, in fact the problem here is just the oposite: lack of standarts.

@PgLoLo
Copy link

PgLoLo commented Aug 15, 2019

Hi,

I am complaining about this PR not being merged. Absence of pyproject.toml support is just nonsense.

@flying-sheep
Copy link

flying-sheep commented Nov 15, 2019

Since the corresponding issue #5205 has been reopened, can we reopen this too?

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 5, 2019

I've now reopened this, but since it's quite a while since this was created, there are a bunch of conflicts that need to be resolved.

@pslacerda
Copy link
Author

pslacerda commented Dec 8, 2019

I think it is done but I'm unsure how to test properly. It was tested with the file that follows and seems to work well. If looks ok I do more rigorous tests.

[mypy]
allow_redefinition = true
python_version = '2.7'

[mypy."package.*.a"]
python_version = '3.9'
allow_untyped_globals = true

@pslacerda pslacerda changed the title Add support to configure mypy with pyproject.toml Add support to pyproject.toml Dec 8, 2019
@pslacerda
Copy link
Author

Maybe instead of deprecate mypy.ini and setup.cfg it should have the following priority.

  1. pyproject.toml
  2. $XDG_CONFIG_HOME/mypy.toml
  3. $HOME/.mypy.toml
  4. mypy.ini
  5. setup.cfg
  6. $XDG_CONFIG_HOME/mypy.ini
  7. $HOME/.mypy.ini

@layday
Copy link

layday commented Dec 8, 2019

(Though I don't regularly contribute to this project I thought I'd chime in here - let me know if I'm out of line.)

You're not going to want to rejig the priority list if you want this to be backwards compatible - keep everything the way it is now with the addition of pyproject.toml either above or below setup.cfg, i.e.:

  1. --config-file
  2. $PWD/mypy.ini
  3. $PWD/setup.cfg
  4. $PWD/pyproject.toml
  5. $XDG_CONFIG_HOME/mypy/config
  6. $HOME/.config/mypy/config
  7. $HOME/.mypy.ini

As a rule of thumb, project configuration should always come before user configuration.

[mypy."package.*.a"]

You should use a sub-table for per-module configuration - the way it's set up now would have options potentially conflict with module names. To illustrate:

>>> toml.loads('''\
... [mypy]
... python_version = "2.7"
...
... [mypy."python_version"]
... allow_untyped_globals = true
... ''')
---------------------------------------------------------------------------
TomlDecodeError                           Traceback (most recent call last)
  ...
TomlDecodeError: What? python_version already exists?{'python_version': '2.7'} (line 4 column 1 char 31)

You can work around this easily by defining per-module configuration in e.g. mypy.overrides:

[mypy]
allow_redefinition = true
python_version = "2.7"

[mypy.overrides."package.*.a"]
python_version = "3.9"
allow_untyped_globals = true

@pslacerda
Copy link
Author

pslacerda commented Dec 8, 2019

Its sunday and I'm specially lay after lunch.

I missed that. Is better [mypy.overrides."package.*.a"] or the following is doable?

[mypy]
python_version = "3.9"
[mypy."@python_version"]
allow_untyped_globals = true

@layday
Copy link

layday commented Dec 8, 2019

Well, TOML has namespacing built into the language, so why would you resort to prefixing modules with at signs? .overrides (or .module_overrides or whatever you might want to call it) is easier for users to grok - the at sign is opaque - and easier for developers to operate on in Python.

@pslacerda
Copy link
Author

pslacerda commented Dec 17, 2019

I think that it is done for now because python runtests.py say it is ok.

Maybe change some tests to use pyproject.toml could help.

EDIT: I also noted that Appveyor's Environment: PYTHON=C:\Python37, PYTHON_VERSION=3.7.x, PYTHON_ARCH=32, EXTRA_ARGS=mypyc/test/test_run.py mypyc/test/test_external.py had previously passed while the PYTHON_ARCH=64 didn't.

Copy link
Collaborator

@ethanhs ethanhs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of smaller pieces of initial feedback, I'll read through the toml parsing code tomorrow.

mypy/config_parser.py Outdated Show resolved Hide resolved
mypy/config_parser.py Outdated Show resolved Hide resolved
mypy/defaults.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ethanhs ethanhs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more nit, and I have several test suggestions:

  • a test that uses multiple options
  • a test for the type converters (this can probably be put in one test)
  • a test for a missing mypy section

Thank you for working on this!

mypy/config_parser.py Outdated Show resolved Hide resolved
@pslacerda
Copy link
Author

pslacerda commented Dec 19, 2019

One more nit, and I have several test suggestions:

One more nigth of my life hunting nits off my hair.

* a test that uses multiple options

* a test for the type converters (this can probably be put in one test)

Ok, I got it.

* a test for a missing mypy section

This one is pickier. Should an empty mypy.ini file (or with no mypy section) be skipped like any other config file?

EDIT: forget about my last point, I'll just keep as it was and test it.

EDIT: Would be a lot easier just change old tests like testMultipleGlobConfigSection ensure everything pass and deprecate .ini files.

Copy link
Author

@pslacerda pslacerda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time for mypy right in the holidays!
@layday would very be welcomed to appear, solve any friction and go back to vacancy again. =)

Happy holidays!

mypy/config_parser.py Show resolved Hide resolved
[out]
main.py:1: error: unused 'type: ignore' comment

[case testMissingMypySection]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But added this test.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think it may be best not to do this. It would be annoying to have mypy fail all of a sudden on projects with a pyproject.toml just because they don't have a mypy section.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't figure this out.

[case testNoConfigFile]
# cmd: mypy main.py --config-file=
[file mypy.ini]
\[mypy]
warn_unused_ignores = True
[file main.py]
# type: ignore

The filename is empty '' so there are two options: try to parse it or go further to default files. Both give different output than expected.

I'm wondering how it even worked:

def parse_config_file(options: Options, filename: Optional[str],

[file main.py]
# type: ignore
[out]
pyproject.toml: No [mypy] table in config file
== Return code: 0

[case testPerFileConfigSection]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And changed this one because to increase TOML coverage. Was it right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer you create a separate test here if possible.

@pslacerda pslacerda force-pushed the feature/pep518 branch 2 times, most recently from e68dac4 to 15f2f59 Compare February 29, 2020 21:16
@pslacerda
Copy link
Author

@erikkemperman I just added your remote and hard reset into your branch, but a force push was needed. No idea how git works.

About replicating the tests from INI I'm almost sure it isn't needed. Many command line tests are done through INI files. Command line, INI or TOML are only parsers, the option handling is the same. The TOML parser is currently tested.

About the disallow/allow thing I'm not sure, my opinion is to keep it off. Are you sure is required?

@erikkemperman
Copy link
Contributor

Yes, the history of the branch changed, relative to the one of the PR previously, due to the rebase onto latest master. That requires a force-push. That would be bad for branches other users might be using, but is usually fine for PRs (although reviewers might have to adjust if they had the changes checked out locally).

Personally I would think the processing of the TOML file should allow pretty much for the same content that is currently allowed in INI files, so I would think that includes this inversion. But perhaps that’s just me — I’d be curious what the maintainers have to say about that?

Same for testing extensively, I see what you mean about it not adding that much in the way of coverage — but for example this issue with inversions would have probably showed up if the tests were all duplicated. But I understand it would be a lot of work... I wonder if there’s anything that could be done to automate this, perhaps in parsing the test cases. I might have some time to take a look later today, if you like?

@pslacerda
Copy link
Author

Some copy & paste and search & replace would alleviate to replicate the tests. I'd love if you @erikkemperman take care of this and help on this boring & interesting PR. But the CLI and INI parsers already share most of the tests, I'm not sure if I'm understanding the need. The only things specific TOML besides general parsing are the bit about strict parsing and the boolean inversion.

Just like the CLI parser I think that boolean inversion is unecessary in the TOML handler. Maybe simply print an error message for any allow/disallow option. I may be wrong of course.

Later this week I'll test strict parsing.

@erikkemperman
Copy link
Contributor

Sorry for late reply.

I actually managed to hack the test-gathering code to duplicate, on the fly, all the ones with an equivalent toml file. Definitely not something to be included in the actual PR, but it gives some confidence that I could get everything to pass:

https://github.com/erikkemperman/mypy/tree/pep518-tweak

I personally would like the inversion to work, and I can imagine the maintainers might get a lot of question if something that worked in INI doesn't work in TOML. But again, maybe that is just me!

While I was rebasing I actually had to account for some strict handling already:

https://github.com/python/mypy/pull/5208/files#diff-09533a8b2bebadfae430681eb15b01c9R178-R179

I just wasn't sure if there needs to be anything for this in the overrides processing as well?

@kudlatyamroth
Copy link

@erikkemperman @pslacerda
is there any update on this?
I think that few ppl wait for it ;)

@pslacerda
Copy link
Author

@kudlatyamroth
It was working as expected. Maybe it continue to work.

There was some minor disagreement between me and @erikkemperman so I'm awaiting for some regular contributor.

@tgy
Copy link

tgy commented Jun 21, 2020

Looking forward to this merge 👀

@Queuecumber
Copy link

@ethanhs can we get some eyes on this? It's the only thing I use that hasn't switched over and one of only a few major tools that doesnt support pyproject.toml

@TitaniumHocker
Copy link

Are there any updates?

@Queuecumber
Copy link

Just pointing out that pylint, pytest, and isort all have support for pyproject.toml @ethanhs

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I don't really understand why people feel so strongly about pyproject.toml, but they do. I would like to make this PR go away, and it seems the easiest way to do that is probably to get it merged. Sorry for all the feet dragging from the core team here. Let's get this done for 0.800. @pslacerda are you still interested in this?

Here's what I need to merge this:

  • Yes, we absolutely need to support option inversion
  • Error handling; produce an error message on an unknown option and on type errors
  • If pyproject.toml is tried implicitly, do not produce a diagnostic if it is missing tool.mypy
  • Ideally try to share a lot of code between the two systems; right now there is a totally separate toml path that duplicates things handled in the ini path in different ways. This will get a lot more painful when the previous two points are implemented. I am willing to entertain arguments that this is not possible or that the cure would be worse than the disease.
  • Don't remove any existing tests
  • Tests for everything mentioned above and some other things: wildcard sections, strict, at least one of the ones that uses a list. (always_true, maybe). @erikkemperman's automatic testing scheme is cute but doesn't currently work on the cmdline tests, which are the key ones here.

A note about the type error issue: one reason that this is important is that mypyc-compiled code generates runtime type errors. So with the current patch, if you do something like disallow_untyped_defs = 0, you'll get a traceback.

We also need documentation, but I'm OK with that being in a follow-up PR.

@msullivan
Copy link
Collaborator

I took the liberty of rebasing and implementing "If pyproject.toml is tried implicitly, do not produce a diagnostic if it is missing tool.mypy". (I don't plan on picking up any of the other pieces myself.)

@pslacerda
Copy link
Author

I'll take another path. The current code is the way I expect. In this PR there are 3 systems, don't forget about CLI that share all the features except inversion if not mistaken.

I agree about the tests. You could rebase this PR or start a new one. Please support pyproject.toml, it is fancier and stronger.

The only drawback I see is the introduction of a new requirement.

@msullivan
Copy link
Collaborator

Alright, if anybody is excited about picking this up (@erikkemperman?), let me know. Otherwise I'll close the PR in a week or so. (It'll be available for anybody to pick back up still, of course.)

@erikkemperman
Copy link
Contributor

@msullivan I would very much like to see support for this, obviously. However I don’t feel entirely comfortable “taking over” the work of @pslacerda if he is no longer behind it. All I did here was a rebase and playing around with the tests, just hoping to nudge things along.

I would need more time with this, more than I have at the moment, before I’d feel confident submitting anything more than that. I hope you understand.

@pslacerda
Copy link
Author

@erikkemperman I can hold the stack so you can work on top of the branch.

@maxcountryman
Copy link

Has anyone taken this on? It would be great to see support for pyproject.toml so that I can define all these things in one place instead of a in dozen-odd disparate, bespoke configurations.

@Olegt0rr
Copy link

Any news?

@ethanhs
Copy link
Collaborator

ethanhs commented Mar 16, 2021

No one has stepped forward to work on this for a while. I am going to close this PR and direct discussion of plans for pyproject.toml support to the issue #5205

If you would like to pick things up, please leave a comment on the issue. (Please do not ask for updates).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet