Skip to content

Fix parsing of unknown message IDs #6315

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
Apr 14, 2022

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Apr 14, 2022

  • Write a good description on what the PR does.
  • 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

The try...expect should have been placed differently. The test was wrong here as well, "logging-format-interpolation" should be in disable even after the previous message is unknown.

@cdce8p This was caused because in the homeassistant config there is a message that has been removed. This caused us to stop parsing the disable key when we reached it.

Ref #6293.

Closes #4324.

@DanielNoord DanielNoord added Bug 🪲 Configuration Related to configuration labels Apr 14, 2022
@DanielNoord DanielNoord added this to the 2.14.0 milestone Apr 14, 2022
Comment on lines 405 to 407
# pylint: disable-next=fixme
# TODO: Optparse: Raise an informational warning here
pass
Copy link
Member

Choose a reason for hiding this comment

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

What about doing it right now ? I don't like this silent fail, in fact we had a more damaging silent fail before and we've been lucky that Marc signaled the issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure! Drafting this then!

@DanielNoord DanielNoord marked this pull request as draft April 14, 2022 11:00
@@ -1,6 +1,7 @@
{
"functional_append": {
"enable": ["locally-disabled"]
"enable": ["locally-disabled"],
"disable": ["logging-format-interpolation"]
Copy link
Member

Choose a reason for hiding this comment

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

Oups, my bad 😅

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

The PR does resolve the issue!

pass
except exceptions.UnknownMessageError:
# pylint: disable-next=fixme
# TODO: Optparse: Raise an informational warning here
Copy link
Member

Choose a reason for hiding this comment

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

@cdce8p This was caused because in the homeassistant config there is a message that has been removed. This caused us to stop parsing the disable key when we reached it.

Didn't know that. I don't usually check all disable message ids to see if they are still valid.
A warning for it (not just informational) would be nice.

@coveralls
Copy link

coveralls commented Apr 14, 2022

Pull Request Test Coverage Report for Build 2169121063

  • 13 of 13 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 93.375%

Totals Coverage Status
Change from base Build 2168825760: 0.002%
Covered Lines: 15969
Relevant Lines: 17102

💛 - Coveralls

@DanielNoord DanielNoord marked this pull request as ready for review April 14, 2022 11:36
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.

Looks pretty good !

@@ -0,0 +1,3 @@
************* Module {abspath}
Copy link
Member

Choose a reason for hiding this comment

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

This kind of silent fail are a pain to deal with, glad it's fixed !

@cdce8p cdce8p removed their request for review April 14, 2022 19:02
@DanielNoord
Copy link
Collaborator Author

Thanks for the clean up @Pierre-Sassoulas

@DanielNoord DanielNoord merged commit 2d9b278 into pylint-dev:main Apr 14, 2022
@DanielNoord DanielNoord deleted the homeassistant branch April 14, 2022 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 Configuration Related to configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add warning if a message in disable or enable inside configuration does not exists
4 participants