Skip to content

Upgrade pylint to 2.2.2#18750

Merged
balloob merged 7 commits intohome-assistant:devfrom
scop:upgrade-pylint
Dec 6, 2018
Merged

Upgrade pylint to 2.2.2#18750
balloob merged 7 commits intohome-assistant:devfrom
scop:upgrade-pylint

Conversation

@scop
Copy link
Copy Markdown
Member

@scop scop commented Nov 27, 2018

Description:

Upgrade to pylint 2.2.1, address new flagged issues.

http://pylint.pycqa.org/en/latest/whatsnew/changelog.html#what-s-new-in-pylint-2-2

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

@ghost ghost assigned scop Nov 27, 2018
@ghost ghost added the in progress label Nov 27, 2018

def setup_message(self):
"""Prevent print of pyhap setup message to terminal."""
pass
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO we should leave pass as it is. It might not be necessary, but it certainly improves readability.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree. Seeing bare lines after a : gives me the willies.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

FWIW I do not agree. But anyway I don't think there's a clean way to omit just these particular unnecessary passes, it would have to be done to all of them. And I think doing that would probably make things worse also in eyes of some people who would like to keep just the unnecessary passes that would have been removed in this PR. I certainly think that would make things worse overall. So I'm afraid we're a bit stuck with the upgrade here, then.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can still look at upgrading – just disable unnecessary-pass in pylintrc.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that just disabling unnecessary-pass is fine. We might get some extra pass in places it really shouldn't be, but I think that's fine.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Pylint has a lot of opinionate cleanup-like options, it does much more than helps catch common mistakes. Many (most?) of those are enabled for HA, even though what they do have only cosmetic/aesthetic value. Opinions vary whether those are liked or are a good thing, and we've now hit one here where they vary.

As said, I don't personally think that a global unnecessary-pass disable would be a good thing to do, so it wouldn't make sense for me to do that. Others seem to have different opinions on the matter, so I suggest someone first does that along with their reasoning in pylintrc in a separate PR, and afterwards I'll rebase this PR, keeping only the uncontroversial changes. Will take a look at upgrading to 2.2.2 meanwhile.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wish PyLint was a bit more configurable. A config option for unnecessary-pass to not count doc strings as statements when determining if a pass is needed would have helped here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, but then again I don't think there's much to blame them for here -- these passes are unnecessary after all, people are just accustomed to seeing them. I found their flagging and looks after removal somewhat odd at first sight too, but after thinking about it for a bit, this is consistent and justifiable so I've tweaked my personal preference instead.

@scop scop changed the title Upgrade pylint to 2.2.1 Upgrade pylint to 2.2.2 Dec 5, 2018
@balloob balloob merged commit 1be440a into home-assistant:dev Dec 6, 2018
@ghost ghost removed the in progress label Dec 6, 2018
@scop scop deleted the upgrade-pylint branch December 7, 2018 07:34
@balloob balloob mentioned this pull request Jan 10, 2019
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.

5 participants