Skip to content

Fix parsing of unrelated options in tox.ini #6801

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

Merged
merged 2 commits into from
Jun 3, 2022

Conversation

DanielNoord
Copy link
Collaborator

  • Write a good description on what the PR does.
  • Add an entry to the change log describing the change in
    doc/whatsnew/2/2.15/index.rst (or doc/whatsnew/2/2.14/full.rst
    if the change needs backporting in 2.14). If necessary you can write
    details or offer examples on how the new change is supposed to work.
  • If you used multiple emails or multiple names when contributing, add your mails
    and preferred name in script/.contributors_aliases.json

Type of Changes

Type
🐛 Bug fix

Description

Closes #6800

We were parsing all options in tox.ini. With unrecognised-option this was now showing up as an error, but the actual fix is to no longer parse all options unless they are under [pylint].

@aneuway2 would you be able to test this fix?

@DanielNoord DanielNoord added Configuration Related to configuration Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer Regression labels Jun 2, 2022
@DanielNoord DanielNoord added this to the 2.14.1 milestone Jun 2, 2022
@coveralls
Copy link

coveralls commented Jun 2, 2022

Pull Request Test Coverage Report for Build 2430451567

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 95.518%

Totals Coverage Status
Change from base Build 2430012610: 0.002%
Covered Lines: 16240
Relevant Lines: 17002

💛 - Coveralls

@github-actions

This comment has been minimized.

DanielNoord referenced this pull request in qiskit-community/quantum-prototype-template Jun 2, 2022
Otherwise, the build warns on Sphinx 5.0.0, and warnings are treated
as errors, so the docs build fails.
@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2022

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉This comment was generated for commit ad86b16

@aneuway2
Copy link

aneuway2 commented Jun 2, 2022

@DanielNoord this looks good on my end (minus some additional linting messages ;-) )

Recap for how I tested in tox:

  1. git clone your repo
  2. checkout the tox branch
  3. pip install $(pwd)
  4. Within [testenv:pylint] I removed pylint from the deps and moved it to whitelist_externals (to get the freshly installed dev build)
  5. Run tox and ignore the error message about using whitelist_externals

@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Jun 2, 2022

@DanielNoord this looks good on my end (minus some additional linting messages ;-) )

Recap for how I tested in tox:

Thanks! That sounds like the right way to test this.

It surprises me this hasn't been caught earlier. It is basically the same issue as #4371 which I considered to be quite problematic. Only this time it is not setup.cfg but tox.ini. I guess there was never a clash in names for tools people configure in tox.ini.

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.

👍

@@ -31,8 +31,7 @@ def __init__(self, verbose: bool, linter: PyLinter) -> None:
self.verbose_mode = verbose
self.linter = linter

@staticmethod
def _parse_ini_file(file_path: Path) -> tuple[dict[str, str], list[str]]:
def _parse_ini_file(self, file_path: Path) -> tuple[dict[str, str], list[str]]:
Copy link
Member

Choose a reason for hiding this comment

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

Tremor of the removal of no-self-use 😄 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes!

Comment on lines +67 to +71
if "setup.cfg" in file_path:
return True
if "tox.ini" in file_path:
return True
return False
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if "setup.cfg" in file_path:
return True
if "tox.ini" in file_path:
return True
return False
return any(n in file_path for n in ["setup.cfg", "tox.ini"])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's leave this as is.

I can imagine we want to expand this. For example, by checking if a section starts with tool:pylint if we are in a setup.cfg file.

@DanielNoord DanielNoord merged commit 05bfb2c into pylint-dev:main Jun 3, 2022
@DanielNoord DanielNoord deleted the tox branch June 3, 2022 06:25
@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 Jun 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pylint 2.14 breaks tox.ini integration with the unrecognized-option error
4 participants