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

Fix recognition of files named setup.cfg (#3630) #6577

Merged
merged 4 commits into from Jun 29, 2022

Conversation

Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented May 11, 2022

Type of Changes

Type
🐛 Bug fix
📜 Docs

Description

Closes #3630. It seems there's still some oddity, init_hook is not treated the same as other option apparentely.

@coveralls
Copy link

coveralls commented May 11, 2022

Pull Request Test Coverage Report for Build 2582528056

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 13 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 95.342%

Files with Coverage Reduction New Missed Lines %
pylint/testutils/_primer/primer_command.py 1 93.75%
pylint/testutils/_primer/primer_run_command.py 12 39.02%
Totals Coverage Status
Change from base Build 2581704758: 0.01%
Covered Lines: 16682
Relevant Lines: 17497

💛 - Coveralls

@@ -0,0 +1 @@
{}
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's it's strange that we have no value set for init-hook

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this should have a value I think. But I'm not 100% sure since this is one of the "pre-processed" options so it might be a bit broken.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Other than the first test (which I agree seems like it is still broken) I don't think the other tests add much value? Those regressions are covered by other tests I think.

If anything, we should probably add comments or change the file names to reflect the issues.

Also note, with the migration to argparse there is now slightly different behaviour between setup.cfg and myconfig.cfg. As for setup we require pylint. section names (although we still support [MASTER]. It might be necessary to use a setup.cfg name instead.

@@ -0,0 +1,3 @@
# Wrong header ?
[tool.pylint.MASTER]
init-hook="from pylint.config import find_pylintrc;"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we use something like print() so we can actually check the init-hook is executed?

@@ -0,0 +1 @@
{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this should have a value I think. But I'm not 100% sure since this is one of the "pre-processed" options so it might be a bit broken.

@@ -0,0 +1,2 @@
************* Module Command line or configuration file
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this testing? This seems a little redundant with all the other tests that check for (incorrect) section headers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just took all the example from the issue :)

@DanielNoord DanielNoord marked this pull request as draft June 21, 2022 07:35
@DanielNoord DanielNoord added Needs review 🔍 Needs to be reviewed by one or multiple more persons Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Jun 29, 2022
@DanielNoord DanielNoord marked this pull request as ready for review June 29, 2022 09:28
@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas This is ready for review!

@DanielNoord DanielNoord modified the milestones: 2.15.0, 2.14.4 Jun 29, 2022
@DanielNoord DanielNoord changed the title Add regression tests for inconsistencies in setup.cfg (#3630) Fix recognition of files named setup.cfg (#3630) Jun 29, 2022
@github-actions

This comment has been minimized.

@@ -0,0 +1,3 @@
I should just print
Copy link
Member Author

Choose a reason for hiding this comment

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

This way of testing the init-hook option is not new, but it's still clever and beautiful 😄

tests/config/functional/setup_cfg/issue_3630/setup.cfg Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas removed the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Jun 29, 2022
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
@Pierre-Sassoulas
Copy link
Member Author

Can't approve the MR I opened myself but let's merge this 👍

@DanielNoord DanielNoord merged commit 63f7c09 into pylint-dev:main Jun 29, 2022
@github-actions
Copy link
Contributor

🤖 Effect of this PR on checked open source code: 🤖

Effect on pandas:
The following messages are now emitted:

  1. unsupported-assignment-operation:
    'mangled_aggspec' does not support item assignment
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/apply.py#L1537
  2. redefined-variable-type:
    Redefinition of mangled_aggspec type from str to list
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/apply.py#L1539
  3. no-member:
    Instance of 'RangeIndex' has no 'levels' member; maybe 'nlevels'?
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/indexes/base.py#L2187
  4. no-member:
    Instance of 'RangeIndex' has no 'codes' member
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/indexes/base.py#L2188
  5. no-member:
    Instance of 'Table' has no 'write_data' member
    https://github.com/pandas-dev/pandas/blob/main/pandas/io/pytables.py#L4367

The following messages are no longer emitted:

  1. no-member:
    Instance of 'Index' has no 'levels' member; maybe 'nlevels'?
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/indexes/base.py#L2187
  2. no-member:
    Instance of 'Index' has no 'codes' member
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/indexes/base.py#L2188

Effect on pytest:
The following messages are now emitted:

  1. consider-using-dict-items:
    Consider iterating with .items()
    https://github.com/pytest-dev/pytest/blob/main/src/_pytest/reports.py#L513
  2. unsubscriptable-object:
    Value 'location' is unsubscriptable
    https://github.com/pytest-dev/pytest/blob/main/src/_pytest/nodes.py#L510

The following messages are no longer emitted:

  1. no-member:
    Class 'Config' has no 'ArgsSource' member
    https://github.com/pytest-dev/pytest/blob/main/src/_pytest/terminal.py#L735

This comment was generated for commit 604deac

Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 29, 2022
…pylint-dev#6577)

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
Pierre-Sassoulas added a commit that referenced this pull request Jun 29, 2022
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
@Pierre-Sassoulas Pierre-Sassoulas deleted the setup-cfg-oddity branch June 29, 2022 14:35
@Pierre-Sassoulas Pierre-Sassoulas added Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some options are not parsed correctly from setup.cfg
3 participants