Skip to content

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
🔨 Refactoring

Description

I wonder if this is considered external. It is in the __all__ but there is no reason for anybody to use this, I think? We don't even use it as the logic for finding the configuration file has moved to find_default_config_files.

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Mar 30, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2065693093

  • 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
  • 8 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.05%) to 94.16%

Files with Coverage Reduction New Missed Lines %
pylint/config/find_default_config_files.py 8 46.3%
Totals Coverage Status
Change from base Build 2064262785: -0.05%
Covered Lines: 15414
Relevant Lines: 16370

💛 - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.0 milestone Mar 30, 2022
@Pierre-Sassoulas Pierre-Sassoulas dismissed their stale review March 30, 2022 16:05

The coverage went down

@Pierre-Sassoulas
Copy link
Member

The coverage decreased because there were tests to check that we could browse a python package to find the pylintrc in parent directory too.

I would be in favor of searching at the root of the project in pyproject.toml, setup.cfg, pylintrc and .pylintrc and only that. It's already a lot more than what flake8 is doing for example. Guess which project has less issues to handle ? 😄 I think it's the way pylint is used most of the time (or should be used). And we added the possibility to do pylint . recently.But it's definitely a breaking change and I also think it's contrary to the per-directory configuration that was a big project of @AWhetter.

This is related to #3769 where they were discussion about configuration.

@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Mar 31, 2022

The coverage decreased because there were tests to check that we could browse a python package to find the pylintrc in parent directory too.

I would be in favor of searching at the root of the project in pyproject.toml, setup.cfg, pylintrc and .pylintrc and only that. It's already a lot more than what flake8 is doing for example. Guess which project has less issues to handle ? 😄 I think it's the way pylint is used most of the time (or should be used). And we added the possibility to do pylint . recently.But it's definitely a breaking change and I also think it's contrary to the per-directory configuration that was a big project of @AWhetter.

This is related to #3769 where they were discussion about configuration.

The issue is that these tests don't test that.. I removed these tests because the only asserts are on find_pylintrc which we don't use internally.
I have a refactor of find_default_config_files ready to go, so I would be more than happy to add similar tests for that function in a follow-up PR.

Edit: Let me rephrase, these tests test that behaviour indirectly. I would like to refactor the function and test directly rather than through find_pylintrc.

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.

I think we should deprecate find_pylintrc first, before removing in 3.0, it's an API. We can add the new tests in this MR too.

@DanielNoord DanielNoord changed the title Remove dead code related to finding the pylintrc file Deprecate find_pylintrc Mar 31, 2022
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.

Lgtm, thank you.

@Pierre-Sassoulas Pierre-Sassoulas merged commit 97abbb8 into pylint-dev:main Mar 31, 2022
@DanielNoord DanielNoord deleted the pylintrc branch March 31, 2022 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants