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 tox.ini to the default discovered config files (right after setup.cfg) #9728

Merged
merged 11 commits into from
Jun 18, 2024

Conversation

mbyrnepr2
Copy link
Member

Type of Changes

Type
βœ“ πŸ› Bug fix
✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Description

Fix a bug where a tox.ini file with pylint configuration was ignored and it exists in the current directory.

Closes #9727

Copy link

codecov bot commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 95.85%. Comparing base (2ba88b9) to head (e0a456d).
Report is 131 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9728   +/-   ##
=======================================
  Coverage   95.85%   95.85%           
=======================================
  Files         174      174           
  Lines       18873    18873           
=======================================
  Hits        18090    18090           
  Misses        783      783           
Files with missing lines Coverage Ξ”
pylint/config/find_default_config_files.py 91.30% <100.00%> (ΓΈ)

... and 2 files with indirect coverage changes

@mbyrnepr2 mbyrnepr2 marked this pull request as ready for review June 12, 2024 14:40
@@ -22,7 +22,8 @@
Path(".pylintrc.toml"),
)
PYPROJECT_NAME = Path("pyproject.toml")
CONFIG_NAMES = (*RC_NAMES, PYPROJECT_NAME, Path("setup.cfg"))
TOX_NAME = Path("tox.ini")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why make this a constant? Can't it be like setup.cfg?

Copy link
Member Author

@mbyrnepr2 mbyrnepr2 Jun 12, 2024

Choose a reason for hiding this comment

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

Thanks @DanielNoord. Agreed.
While I have your attention, what's your opinion on this logic and the associated comments around the inconsistencies mentioned on the issue? It does feel like we should allow [pylint] and not necessarily [pylint.something] but there could be some rationale for that.

Our own documentation has this: setup.cfg in the current working directory, providing it has at least one pylint. section

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we previously only allowed pylint. because the way optparse worked required sections. I removed that when migrating to argparse and improving support for toml.

I would support changing this to be 1. exactly "pylint" or 2. anything that starts with "pylint.". To make sure we are not starting to parse "pylint-internal-plugin" etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI @Pierre-Sassoulas and also may need to wait/further refactor before merging.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the ping :) I think anything starting with pylint. is closer to what's happening in pyproject.toml so I would favor that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In .toml we support [tool.pylint]. I think making .ini in line with this makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Should we make the tool mandatory ? As in [tool.pylint], or [tool.pylint.main], [tool.pylint.basic] (etc.). We don't in ini file currently, but the ini file is pylint specific so it doesn't need the "pylint" / "tool.pylint" part.

[BASIC]

So maybe [pylint], or [pylint.main], [pylint.basic]. for consistency in tox.ini (consistency with file of the same format above consistency with toml), and backward compatibility in ini ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

tool is a thing because of the pyproject.toml spec I think.

Seeing other tox.ini files I think nobody would be surprised that [pylint] would be parsed as the configuration for pylint. We currently require people to do[pylint.some_section_that_isnt_even_validated] which feels cumbersome and inconsistent. That's why I proposed changing it from only [pylint\..+] to also allow [pylint].

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok thanks folks! I've tried to match Daniel's vision with the latest commit; accept [pylint] or [pylint.<something_arbitrary>]. Let me know if I've gotten anything wrong!

@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Jun 12, 2024
@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.3.0 milestone Jun 12, 2024
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas 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 nit then LGTM. I classified as enhancement because it can make pylint start behaving unexpectedly if a tox.ini is present and this should probably not happen in a patch. (We can release 3.3.0 fast however).

tests/config/test_find_default_config_files.py Outdated Show resolved Hide resolved
tests/config/test_find_default_config_files.py Outdated Show resolved Hide resolved
mbyrnepr2 and others added 4 commits June 12, 2024 21:36
@mbyrnepr2 mbyrnepr2 force-pushed the tox_ini_default_config_file branch from 75580c9 to 6aeba71 Compare June 12, 2024 21:24
@rlalik
Copy link

rlalik commented Jun 13, 2024

Hi, original issue reporter here.

Upon request of @mbyrnepr2 in #9727 I tested the PR-version and it works fine for my case for both tox.ini and setup.cfg with proper config files autodetection.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Great πŸ‘Œ

@@ -0,0 +1,3 @@
Fix a bug where a ``tox.ini`` file with pylint configuration was ignored and it exists in the current directory.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably include the change about accepted sections here as well.

@@ -71,7 +74,9 @@ def _yield_default_files() -> Iterator[Path]:
if config_name.is_file():
if config_name.suffix == ".toml" and not _toml_has_config(config_name):
continue
if config_name.suffix == ".cfg" and not _cfg_has_config(config_name):
if config_name.suffix in {".cfg", ".ini"} and not _cfg_has_config(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if config_name.suffix in {".cfg", ".ini"} and not _cfg_has_config(
if config_name.suffix in {".cfg", ".ini"} and not _cfg_or_ini_has_config(

mbyrnepr2 and others added 2 commits June 17, 2024 16:03
configuration section naming for `.cfg` and `.ini` files.
Copy link
Contributor

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit e0a456d

@DanielNoord DanielNoord merged commit fc9bdeb into pylint-dev:main Jun 18, 2024
44 checks passed
@mbyrnepr2 mbyrnepr2 deleted the tox_ini_default_config_file branch June 18, 2024 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pylint not finding the tox.ini config
4 participants