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

Mix incorrect parsing of multi-line options in ini files #6944

Merged
merged 1 commit into from
Jun 13, 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 #6888.

We could also use an if statement to check whether the option is init-hook and and then not strip the newlines. But that introduce additional overhead, while this makes pylint faster. The fact that the rest of the test suite passes makes me think that this will be fine. We strip most options of whitespaces in other places anyway.

If this regresses after 2.14.2 we can simply add the if statement as desired.

@DanielNoord DanielNoord added Bug 🪲 Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer Regression labels Jun 13, 2022
@DanielNoord DanielNoord added this to the 2.14.2 milestone Jun 13, 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.

I think this king of option is impossible to test with the config functional test framework.

@@ -0,0 +1,6 @@
# Reported in https://github.com/PyCQA/pylint/issues/6888
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the result.json change ? Probably not as init_hook are treated as a special case before the config is instanced, so we should probably create a unit test with multiple print so we can actually check that the code of each line was executed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This raises a SyntaxError and fails the test without the changes in the PR. So it is actually a test against the reported issue: multi-line init hooks became syntactically incorrect code.

@DanielNoord DanielNoord merged commit acc724b into pylint-dev:main Jun 13, 2022
@DanielNoord DanielNoord deleted the init-hook branch June 13, 2022 16:24
@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 55e8a35

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2489188691

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.0003%) to 95.541%

Totals Coverage Status
Change from base Build 2489123584: -0.0003%
Covered Lines: 16411
Relevant Lines: 17177

💛 - Coveralls

@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 15, 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.

init-hook no longer supports multi-line hook
3 participants