-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: Allow all config formats in all config locations #3769
Conversation
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 think this is a welcome change, and a very important one, but also a very sensible one that could make a lot of CI/run fails if not done correctly. Could you add tests for this, please ? In particular for the get_config_path function. I'm also going to wait for another review before merging it for this reason.
116425b
to
d573222
Compare
I rebased onto master, added typing, and added a test case. Thanks for the input. Let me know if I need more tests or can improve in another way. |
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.
Thank you for taking my review into account. I don't have enough time right now to check that it's not breaking the current expected order of configuration file priority, Please don't hesitate to ping me if I still did not review it properly by october.
The only place it may break current order of priority is in the home folder. Previously it only looked for |
dd79745
to
e7a79bf
Compare
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 for the long time between first fast review and actual review. I don't see a problem here but I'm going to wait for another review before putting it in 2.7.0.
Sounds good, just let me know of any feedback and/or changes needed :) |
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.
@danrneal thanks for this clear and welcomed PR!
I just have few comments/questions.
#. ``pylintrc`` | ||
#. ``.pylintrc`` | ||
#. ``pyproject.toml``, provided it has at least one ``tool.pylint.`` section. | ||
#. ``setup.cfg``, provided it has at least one ``pylint.`` section. |
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 happens if two or more valid files are found in the same location?
Maybe the doc should explain this.
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 added that they were listed in order or priority. So if .pylintrc and setup.cfg are in the same location it'll pick .pylintrc. Is that clearer or do I need to expand more than just the note?
|
||
if os.path.isfile("/etc/pylintrc"): | ||
yield "/etc/pylintrc" | ||
|
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.
Why did the else
statement disappear?
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.
Isn't the else is redundant when there is a yield or return statement inside and if block?
I'm moving this to 2.7.1, I think it's addable as a minor feature later. |
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 to jump in so late in the game. I don't think that it makes much sense to look for a pyproject.toml or a setup.cfg anywhere other than in the project because those files store metadata about a specific project. It's in the name of the pyproject file and in the PEP of the file (https://www.python.org/dev/peps/pep-0621/). Those files don't really belong outside of a project, so I can't think of a good reason to look for them anywhere other than the current directory.
Looking for a .pylintrc anywhere other than in the currently working directory also seems out of place. It makes sense to allow it in the current working directory because maybe you want to obscure it from view to keep the project tree looking tidy. But the directories ~/.config and /etc are the places for config files, so I can't think of a reason why you would want to obscure a config file from view in those places.
Side note: For a while I've been feeling like the config system in pylint is not great and it needs a bit of attention. The code itself is pretty unwieldy and I think the user experience of configuration isn't great either. Changes like this make me nervous because some day we'll want to give the configuration the attention it deserves, but that means making breaking changes. So I'd rather not expand the bounds of the configuration system too much so that aren't breaking as much for people later on and so we can focus more on what the config system should do, rather than needing to worry about what we might break.
@AWhetter I agree that the configuration need to be redesigned and that we don't want to have too much to break when we do that. I also agree pyproject.toml or setup.cfg makes no sense outside of the current project directory and you convinced me we should not start maintaining and documenting things that do not make sense (especially considering the fact that we had two possible names for pylintrc already But isn't
|
Yes absolutely! I like both of these places for a pylintrc file. But not for |
I think the |
Yes that's what I meant. I definitely think that config inheritance is something that we should consider in the future, but we shouldn't implement it now. |
Both isort and black stop going up directories when they get to a .git or .hg directory. But the use case for having a single config for isort, pylint, and black in the home directory is for coding that isn't part of a project. If you had a global default pyproject.toml then your code can still be linted and checked for style even if your code is not part of a project. I can address the comments in the PR and get it back into ship shape. However, if this isn't the direction to go in, then there probably isn't a lot of value in me doing that. I'll wait for more discussion. |
I think we could mirror what isort and black do, what do you think @AWhetter ? Maybe we need to refactor this for 3.0 as a real breaking change ? |
I'm going to close as we're going to move to non global configuration files. Thank you for your contribution and sorry for being indecisive @danrneal |
Steps
doc/whatsnew/<current release.rst>
.Description
Type of Changes
This PR would allow a
pylintrc
,.pylintrc
,pyproject.toml
, orsetup.cfg
in any location that config files are stored. For example, currentlypyproject.toml
will only be read from the project root but not from the home directory. This PR would allow it to be read from the home directory. This is helpful when creating a global default config as other projects (such as isort) allow this config format in the home director. Let me know if this is a welcome change with or without revision or an unwelcome one.