Add support for additional predefined config file paths #2761

Merged
merged 1 commit into from Feb 14, 2017

Conversation

Projects
None yet
6 participants
@miedzinski
Contributor

miedzinski commented Jan 26, 2017

When --config-file option isn't passed to mypy and mypy can't load mypy.ini (default config filename), it will try to load settings from common shared config files -- currently setup.cfg and tox.ini. However, with these files, mypy will become less verbose; specifically it won't require mypy section.

My main incentive is that I don't wan't to "bloat" my project structure with loads of config files; OTOH I expect them to work just by typing well-known command (with options loaded from configuration files). Two common development tools I use are pytest and flake8 and both support loading setup.cfg files. Another popular one is tox, which unfortunately doesn't load that file, but the former two load tox.ini in addition to setup.cfg. Moreover, tox is often used to run all these tools with mypy included and I believe some people consider keeping all configuration in one place as a nice thing to have.

I do understand that this is kind of an anti-pattern, but with small codebases it is quite annoying to have as many config files as source code. This is why it is implemented as a completely optional feature, with --config-file option and mypy.ini taking precedence over shared configs. Anyway, this PR is rather small - I hope the code won't need any maintenance - and some people will benefit from it, while others will never use it.

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Jan 27, 2017

Collaborator

Hi! Thanks for submitting this PR.

If I recall correctly, when implementing config files initially, we considered using setup.cfg and tox.ini as fallbacks and decided against it. I don't remember the rationale, though -- @gvanrossum, do you remember?

Collaborator

ddfisher commented Jan 27, 2017

Hi! Thanks for submitting this PR.

If I recall correctly, when implementing config files initially, we considered using setup.cfg and tox.ini as fallbacks and decided against it. I don't remember the rationale, though -- @gvanrossum, do you remember?

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jan 27, 2017

Member

I prefer not to do this. To explain my reasoning (then and now): as a consumer of projects created by others I've often been confused by the rules for finding configuration data in seemingly randomly-named files like tox.ini (I've never heard of anyone who uses it) and setup.cfg, combined with directory searches. E.g. if there's a tox.ini in a subdirectory but a setup.cfg in the root directory of a project, which one wins, or are they combined somehow?

Member

gvanrossum commented Jan 27, 2017

I prefer not to do this. To explain my reasoning (then and now): as a consumer of projects created by others I've often been confused by the rules for finding configuration data in seemingly randomly-named files like tox.ini (I've never heard of anyone who uses it) and setup.cfg, combined with directory searches. E.g. if there's a tox.ini in a subdirectory but a setup.cfg in the root directory of a project, which one wins, or are they combined somehow?

@miedzinski

This comment has been minimized.

Show comment
Hide comment
@miedzinski

miedzinski Jan 27, 2017

Contributor

@ddfisher, @gvanrossum,

I understand and this is something I've expected. I think my reasoning can be explained when we look at hypothetical open-source Python project, which decides to incorporate quite a few modern tools:

  • flake8 - code linting
  • pytest - test framework
  • coverage.py - code coverage analysis
  • mypy - type checking
  • tox - virtualenv management and test runner combined (by the way it is quite common; e.g. python/asyncio uses it)

This project may also use other tools, which require configuration, but we'll ignore them (for instance it may use something like isort, probably have requirements.txt file, maybe requirements-(dev|test|docs).txt or it uses brand new Pipfile and Pipfile.lock).

Now let's say some new developer decides to fork the repo and hack some things on it. Sooner or later, he will want to test/lint/typecheck/something his code. The project probably has some kind of runtests.py script; since it uses tox running all suites may be as simple as typing tox. However, the developer often wants to just lint his code or typecheck it and he knows that flake8 and mypy do that. In his first try he will try typing these commands (some may try something like tox -e flake8,mypy because it has tox). How can we make sure this will work as expected? By having these tools configured and making sure they pick up configs out of the box (without explicitly passing them as CLI option).

What will be read?

  • flake8 - .flake8, setup.cfg, tox.ini, and config/flake8 on Windows
  • pytest - pytest.ini, tox.ini, setup.cfg
  • coverage.py - .coveragerc, setup.cfg, tox.ini
  • mypy - mypy.ini
  • tox - tox.ini

One file per tool or shared configs. It's possible to stick everything into tox.ini or split into tox.ini and setup.cfg - tox will probably run these tools, which will all read setup.cfg. Often, many projects decide to have dedicated files for some tools, and shared for others (mixing conventions). Take a look at Django: setup.cfg contains flake8 and isort (it uses .isort.cfg by default), .coveragerc (in tests sub-project) and of course tox.ini.

Mypy devs themselves have decided that flake8 and coverage.py can have their configs in setup.cfg, while keeping pytest in pytest.ini. In some sense it is unfair that mypy itself "requires" dedicated file.

Personally, I see (at least) two types of tools:

  • build tools (setuptools - setup.py, pip - requirements.txt, pipenv - Pipfile[.lock])
  • code analysis (flake8, pytest, mypy included)

In the former, I believe they should have only dedicated files (and they do), because these are required to turn source code into a program. The latter should not, since they are only to help and it should be up to developers how to configure them. Since there is visible trend in the community to use setup.cfg, we should do the same.

Why I don't want that many files? Try to write a modern (defined as using FOTM frameworks and build tools) JS app and you'll see. Even django, despite being mostly Python codebase, has four JS-related configs in project root. A lot of people say JS ecosystem has some flaws in it, and, IMO, configs scattered all over the place contribute to it. Again, in my opinion, if an user wants to have mypy configured out of the box in his codebase, we shouldn't force him to create yet another file. If developers want to keep all configs in single place, noone will stop them and they will work around that in either way, e.g. telling contributors to run some tools with --config-file setup.cfg option.

Lastly, regarding subdirectories and merging configs. AFAIK none of these tools do this and neither would mypy after merging this PR. It would look for --config-file option and then, assuming user didn't specify CLI option, read well-known files in project root in order:

  1. mypy.ini
  2. setup.cfg
  3. tox.ini - to be honest I don't like this one and was thinking on whether it should be read; decided to add it because other tools do so

If mypy manages to read some file, it stops reading next. This why there won't be any file merging; I think noone does it and it'd be a hell of a nightmare to manage. Additionally - for already configured projects - if you have mypy.ini and/or no mypy related entries in shared configs, mypy won't change its configuration.

EDIT:
I've misunderstood Guido in regard to tox. I thought you don't know anyone using tox - pardon me. Like I said above, I agree that tox.ini may not be the best file to read config from and I could accept dropping it from this PR.

Contributor

miedzinski commented Jan 27, 2017

@ddfisher, @gvanrossum,

I understand and this is something I've expected. I think my reasoning can be explained when we look at hypothetical open-source Python project, which decides to incorporate quite a few modern tools:

  • flake8 - code linting
  • pytest - test framework
  • coverage.py - code coverage analysis
  • mypy - type checking
  • tox - virtualenv management and test runner combined (by the way it is quite common; e.g. python/asyncio uses it)

This project may also use other tools, which require configuration, but we'll ignore them (for instance it may use something like isort, probably have requirements.txt file, maybe requirements-(dev|test|docs).txt or it uses brand new Pipfile and Pipfile.lock).

Now let's say some new developer decides to fork the repo and hack some things on it. Sooner or later, he will want to test/lint/typecheck/something his code. The project probably has some kind of runtests.py script; since it uses tox running all suites may be as simple as typing tox. However, the developer often wants to just lint his code or typecheck it and he knows that flake8 and mypy do that. In his first try he will try typing these commands (some may try something like tox -e flake8,mypy because it has tox). How can we make sure this will work as expected? By having these tools configured and making sure they pick up configs out of the box (without explicitly passing them as CLI option).

What will be read?

  • flake8 - .flake8, setup.cfg, tox.ini, and config/flake8 on Windows
  • pytest - pytest.ini, tox.ini, setup.cfg
  • coverage.py - .coveragerc, setup.cfg, tox.ini
  • mypy - mypy.ini
  • tox - tox.ini

One file per tool or shared configs. It's possible to stick everything into tox.ini or split into tox.ini and setup.cfg - tox will probably run these tools, which will all read setup.cfg. Often, many projects decide to have dedicated files for some tools, and shared for others (mixing conventions). Take a look at Django: setup.cfg contains flake8 and isort (it uses .isort.cfg by default), .coveragerc (in tests sub-project) and of course tox.ini.

Mypy devs themselves have decided that flake8 and coverage.py can have their configs in setup.cfg, while keeping pytest in pytest.ini. In some sense it is unfair that mypy itself "requires" dedicated file.

Personally, I see (at least) two types of tools:

  • build tools (setuptools - setup.py, pip - requirements.txt, pipenv - Pipfile[.lock])
  • code analysis (flake8, pytest, mypy included)

In the former, I believe they should have only dedicated files (and they do), because these are required to turn source code into a program. The latter should not, since they are only to help and it should be up to developers how to configure them. Since there is visible trend in the community to use setup.cfg, we should do the same.

Why I don't want that many files? Try to write a modern (defined as using FOTM frameworks and build tools) JS app and you'll see. Even django, despite being mostly Python codebase, has four JS-related configs in project root. A lot of people say JS ecosystem has some flaws in it, and, IMO, configs scattered all over the place contribute to it. Again, in my opinion, if an user wants to have mypy configured out of the box in his codebase, we shouldn't force him to create yet another file. If developers want to keep all configs in single place, noone will stop them and they will work around that in either way, e.g. telling contributors to run some tools with --config-file setup.cfg option.

Lastly, regarding subdirectories and merging configs. AFAIK none of these tools do this and neither would mypy after merging this PR. It would look for --config-file option and then, assuming user didn't specify CLI option, read well-known files in project root in order:

  1. mypy.ini
  2. setup.cfg
  3. tox.ini - to be honest I don't like this one and was thinking on whether it should be read; decided to add it because other tools do so

If mypy manages to read some file, it stops reading next. This why there won't be any file merging; I think noone does it and it'd be a hell of a nightmare to manage. Additionally - for already configured projects - if you have mypy.ini and/or no mypy related entries in shared configs, mypy won't change its configuration.

EDIT:
I've misunderstood Guido in regard to tox. I thought you don't know anyone using tox - pardon me. Like I said above, I agree that tox.ini may not be the best file to read config from and I could accept dropping it from this PR.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jan 27, 2017

Member
Member

gvanrossum commented Jan 27, 2017

@habnabit

This comment has been minimized.

Show comment
Hide comment
@habnabit

habnabit Jan 27, 2017

I am still not convinced. The desire for fewer files in the project root is honest, but naive. There are going to be tons of files there, like it or not. It's just how the world works.

Given a chance to try to reverse this trend, why not take it?

I am still not convinced. The desire for fewer files in the project root is honest, but naive. There are going to be tons of files there, like it or not. It's just how the world works.

Given a chance to try to reverse this trend, why not take it?

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jan 27, 2017

Member

Given a chance to try to reverse this trend, why not take it?

See my earlier reply. The multiple files are confusing.

Member

gvanrossum commented Jan 27, 2017

Given a chance to try to reverse this trend, why not take it?

See my earlier reply. The multiple files are confusing.

@gvanrossum gvanrossum closed this Jan 27, 2017

@miedzinski

This comment has been minimized.

Show comment
Hide comment
@miedzinski

miedzinski Jan 27, 2017

Contributor

@gvanrossum, please consider reopening. The actual discussion has just begun to appear. I've brought up a topic on Python tools about using setup.cfg on freenode's #python IRC channel and, from what I see, people seem to be happy about it.

I'd be cautious when saying there will be tons of files anyway. I am using pytest, flake8, and mypy in my project and with this PR I can halve config files amount - to one.

The rules for precedence are easy:

  1. read --config-file option - if it's incorrect, exit
  2. read mypy.ini - if correct, stop
  3. read setup.cfg

Subdirectory behavior is clear as well - mypy doesn't traverse directory tree looking for mypy.ini and it wouldn't do so to find setup.cfg. Pytest behavior is irrelevant here.

By the way, would you close PR with support for PEP 512? Like it or not, the community (at least some part of it) has agreed on setup.cfg. Until PEP 512 becomes widely adopted, there is nothing you can do. I really dislike that mypy wants to be a special snowflake and force users to create a dedicated file, usually with only few lines in it.

And please don't say that setup.cfg would be confusing. That's nonsense. If an user writes mypy options there, it won't be surprising if mypy reads them. I'm pretty sure a lot of Python developers know about setup.cfg purpose, because they configure a lot of tools there. Remember, noone forces anyone to use it - mypy.ini should be preferred, and with this PR, it is.

Additionally, after @nedbat suggestion, I've dropped tox.ini support. This is something I agree with, but was included in just because other tools do so.

EDIT:
Here is the tox.ini commit - miedzinski/mypy@3240219. Looks like GitHub won't pick it up until reopened.

Contributor

miedzinski commented Jan 27, 2017

@gvanrossum, please consider reopening. The actual discussion has just begun to appear. I've brought up a topic on Python tools about using setup.cfg on freenode's #python IRC channel and, from what I see, people seem to be happy about it.

I'd be cautious when saying there will be tons of files anyway. I am using pytest, flake8, and mypy in my project and with this PR I can halve config files amount - to one.

The rules for precedence are easy:

  1. read --config-file option - if it's incorrect, exit
  2. read mypy.ini - if correct, stop
  3. read setup.cfg

Subdirectory behavior is clear as well - mypy doesn't traverse directory tree looking for mypy.ini and it wouldn't do so to find setup.cfg. Pytest behavior is irrelevant here.

By the way, would you close PR with support for PEP 512? Like it or not, the community (at least some part of it) has agreed on setup.cfg. Until PEP 512 becomes widely adopted, there is nothing you can do. I really dislike that mypy wants to be a special snowflake and force users to create a dedicated file, usually with only few lines in it.

And please don't say that setup.cfg would be confusing. That's nonsense. If an user writes mypy options there, it won't be surprising if mypy reads them. I'm pretty sure a lot of Python developers know about setup.cfg purpose, because they configure a lot of tools there. Remember, noone forces anyone to use it - mypy.ini should be preferred, and with this PR, it is.

Additionally, after @nedbat suggestion, I've dropped tox.ini support. This is something I agree with, but was included in just because other tools do so.

EDIT:
Here is the tox.ini commit - miedzinski/mypy@3240219. Looks like GitHub won't pick it up until reopened.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jan 27, 2017

Member

I'll reopen, but I will continue to argue against. Look at https://github.com/python/mypy, the toplevel has already 18 different files, a majority of which are configuration of some sort or other.

Maybe @ambv has an opinion? He's got more experience with a variety of tools like this than I have. (And I notice that he added a .flake8 file to typeshed rather than a setup.cfg.)

Member

gvanrossum commented Jan 27, 2017

I'll reopen, but I will continue to argue against. Look at https://github.com/python/mypy, the toplevel has already 18 different files, a majority of which are configuration of some sort or other.

Maybe @ambv has an opinion? He's got more experience with a variety of tools like this than I have. (And I notice that he added a .flake8 file to typeshed rather than a setup.cfg.)

@gvanrossum gvanrossum reopened this Jan 27, 2017

docs/source/command_line.rst
- and command line flags can override settings. See :ref:`config-file`
- for the syntax of configuration files.
+ read from the given file. By default settings are read from ``mypy.ini``,
+ ``setup.cfg``, or ``tox.ini`` in the current directory. Settings override

This comment has been minimized.

@gvanrossum

gvanrossum Jan 27, 2017

Member

You still mention tox.ini here (and several other places). Also you should clarify that it stops at the first file it finds (here and in the next chapter).

@gvanrossum

gvanrossum Jan 27, 2017

Member

You still mention tox.ini here (and several other places). Also you should clarify that it stops at the first file it finds (here and in the next chapter).

@illume

This comment has been minimized.

Show comment
Hide comment
@illume

illume Feb 7, 2017

I see flake8 and converage config are in the setup.cfg file now at https://github.com/python/mypy

Note that pytest can also use setup.cfg (and tox.ini). See how they do it here: http://doc.pytest.org/en/latest/customize.html

Some packages are really drowning in config file soup, with 20-30 config files! Hopefully at least all the python tools can agree on a common file for config.

illume commented Feb 7, 2017

I see flake8 and converage config are in the setup.cfg file now at https://github.com/python/mypy

Note that pytest can also use setup.cfg (and tox.ini). See how they do it here: http://doc.pytest.org/en/latest/customize.html

Some packages are really drowning in config file soup, with 20-30 config files! Hopefully at least all the python tools can agree on a common file for config.

@ambv

This comment has been minimized.

Show comment
Hide comment
@ambv

ambv Feb 13, 2017

Contributor

Sorry, didn't notice the @ until now. I added a .flake8 rather than a setup.cfg because there's no setup.py so using setup.cfg felt wrong.

That being said, I don't think additional support for setup.cfg configuration would be confusing. I imagine that the general rules would be that:

  • --config-file= trumps all
  • within a directory, mypy.ini trumps setup.cfg
  • no overriding of config in sub-directories
  • no combining of multiple sources of config

With those explicitly stated in the docs, I think there's little space for surprise on the user's end.

Oh, and I agree that tox.ini can be ignored.

Contributor

ambv commented Feb 13, 2017

Sorry, didn't notice the @ until now. I added a .flake8 rather than a setup.cfg because there's no setup.py so using setup.cfg felt wrong.

That being said, I don't think additional support for setup.cfg configuration would be confusing. I imagine that the general rules would be that:

  • --config-file= trumps all
  • within a directory, mypy.ini trumps setup.cfg
  • no overriding of config in sub-directories
  • no combining of multiple sources of config

With those explicitly stated in the docs, I think there's little space for surprise on the user's end.

Oh, and I agree that tox.ini can be ignored.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Feb 13, 2017

Member
Member

gvanrossum commented Feb 13, 2017

@miedzinski

This comment has been minimized.

Show comment
Hide comment
@miedzinski

miedzinski Feb 13, 2017

Contributor

It is exactly as @ambv described.

Contributor

miedzinski commented Feb 13, 2017

It is exactly as @ambv described.

@gvanrossum

Bunch of style nits. We're almost there!

docs/source/command_line.rst
@@ -344,10 +344,10 @@ Here are some more useful flags:
.. _config-file-flag:
- ``--config-file CONFIG_FILE`` causes configuration settings to be
- read from the given file. By default settings are read from ``mypy.ini``

This comment has been minimized.

@gvanrossum

gvanrossum Feb 13, 2017

Member

Please put the double spaces after periods back. They don't affect how the HTML is rendered, but they improve the quality of paragraph reflowing in Emacs.

@gvanrossum

gvanrossum Feb 13, 2017

Member

Please put the double spaces after periods back. They don't affect how the HTML is rendered, but they improve the quality of paragraph reflowing in Emacs.

docs/source/command_line.rst
- and command line flags can override settings. See :ref:`config-file`
- for the syntax of configuration files.
+ read from the given file. By default settings are read from ``mypy.ini``
+ and ``setup.cfg`` in the current directory. Settings override mypy's

This comment has been minimized.

@gvanrossum

gvanrossum Feb 13, 2017

Member

maybe "and" -> "or" ?

@gvanrossum

gvanrossum Feb 13, 2017

Member

maybe "and" -> "or" ?

docs/source/config_file.rst
+read a different file instead (see :ref:`--config-file <config-file-flag>`).
+
+It is important to understand that there is no merging of configuration
+files, as it would lead to disambiguity. The ``--config-file`` flag

This comment has been minimized.

@gvanrossum

gvanrossum Feb 13, 2017

Member

drop "dis"

@gvanrossum

gvanrossum Feb 13, 2017

Member

drop "dis"

This comment has been minimized.

@miedzinski

miedzinski Feb 13, 2017

Contributor

I'm not sure if I understood this correctly. You meant first sentence only, right?

@miedzinski

miedzinski Feb 13, 2017

Contributor

I'm not sure if I understood this correctly. You meant first sentence only, right?

This comment has been minimized.

@gvanrossum

gvanrossum Feb 13, 2017

Member

I actually just meant to change "disambiguity" to "ambiguity". What you wrote was backwards. :-)

@gvanrossum

gvanrossum Feb 13, 2017

Member

I actually just meant to change "disambiguity" to "ambiguity". What you wrote was backwards. :-)

This comment has been minimized.

@miedzinski

miedzinski Feb 14, 2017

Contributor

Oh, you're absolutely right. Restored the sentence.

@miedzinski

miedzinski Feb 14, 2017

Contributor

Oh, you're absolutely right. Restored the sentence.

mypy/main.py
"""Parse a config file into an Options object.
Errors are written to stderr but are not fatal.
+
+ If filename isn't provided, fall back to default config file and then

This comment has been minimized.

@gvanrossum

gvanrossum Feb 13, 2017

Member

"isn't provided" -> "is None"

@gvanrossum

gvanrossum Feb 13, 2017

Member

"isn't provided" -> "is None"

mypy/main.py
"""Parse a config file into an Options object.
Errors are written to stderr but are not fatal.
+
+ If filename isn't provided, fall back to default config file and then
+ to common shared files (currently setup.cfg).

This comment has been minimized.

@gvanrossum

gvanrossum Feb 13, 2017

Member

I would just say "to setup.cfg". If and when we change it we can update the comment.

@gvanrossum

gvanrossum Feb 13, 2017

Member

I would just say "to setup.cfg". If and when we change it we can update the comment.

mypy/main.py
+ if filename is not None:
+ config_files = (filename, )
+ else:
+ config_files = (defaults.CONFIG_FILE, ) + SHARED_CONFIG_FILES

This comment has been minimized.

@gvanrossum

gvanrossum Feb 13, 2017

Member

No space between "," and ")". (Also two lines up.)

@gvanrossum

gvanrossum Feb 13, 2017

Member

No space between "," and ")". (Also two lines up.)

mypy/main.py
@@ -540,31 +537,52 @@ def get_init_file(dir: str) -> Optional[str]:
'almost_silent': bool,
}
+SHARED_CONFIG_FILES = ('setup.cfg', )

This comment has been minimized.

@gvanrossum

gvanrossum Feb 13, 2017

Member

No space between "," and ")".

@gvanrossum

gvanrossum Feb 13, 2017

Member

No space between "," and ")".

@miedzinski

This comment has been minimized.

Show comment
Hide comment
@miedzinski

miedzinski Feb 13, 2017

Contributor

Fixed. Please forgive my English - I'm trying to do my best despite it's not my native language. And I'm more of a Vim guy. :)

Contributor

miedzinski commented Feb 13, 2017

Fixed. Please forgive my English - I'm trying to do my best despite it's not my native language. And I'm more of a Vim guy. :)

Read configuration from setup.cfg by default
When --config-file option isn't passed to mypy and mypy can't load
mypy.ini (default config filename), it will try to load settings
from setup.cfg.

@gvanrossum gvanrossum merged commit 70b278c into python:master Feb 14, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Feb 14, 2017

Member

Thanks!

PS. Next time can you not squash your commits? It's easier to go through review cycles if every push is a separate commit. e squash and rewrite the commit message when we merge PRs anyways.

Member

gvanrossum commented Feb 14, 2017

Thanks!

PS. Next time can you not squash your commits? It's easier to go through review cycles if every push is a separate commit. e squash and rewrite the commit message when we merge PRs anyways.

@matrixik matrixik referenced this pull request in PyCQA/pylint Oct 26, 2017

Closed

search pylint configuration in setup.cfg too #617

@Flimm Flimm referenced this pull request in jazzband/sorl-thumbnail Nov 22, 2017

Closed

Create a tox.ini file #525

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment