Skip to content

Upgrade pylint to 1.8.2#12274

Merged
balloob merged 26 commits into
home-assistant:devfrom
OttoWinter:upgrade-pylint
Feb 11, 2018
Merged

Upgrade pylint to 1.8.2#12274
balloob merged 26 commits into
home-assistant:devfrom
OttoWinter:upgrade-pylint

Conversation

@OttoWinter
Copy link
Copy Markdown
Member

@OttoWinter OttoWinter commented Feb 10, 2018

Description:

This PR upgrades pylint to 1.8.2 and fixes all resulting error checks.

Related issue (if applicable): #7108

bad-whitespace

PEP8 now recommends whitespace between an argument annotation and a default value around the = sign (see pylint-dev/pylint#238)

When combining an argument annotation with a default value, use spaces around the = sign (but only for those arguments that have both an annotation and a default).
Yes:

def munge(sep: AnyStr = None): ...
def munge(input: AnyStr, sep: AnyStr = None, limit=1000): ...

No:

def munge(input: AnyStr=None): ...
def munge(input: AnyStr, limit = 1000): ...

The code update however causes pylint 1.6.5 to complain about the version with the whitespace (i.e. the correct version) - so either the commit has to be reverted (and bad-whitespace needs to be disabled for 1.8.2) or bad-whitespace is disabled for pylint 1.6.5, which could potentially be a bad idea.

arguments-differ

Makes sure that the arguments for a overridden method match the super class. Also checks whether the names of the arguments match, which seems a bit verbose to me... Anyway, there's one problem though: DeviceScanner::get_device_name(self, mac: str) -> str should, as far as I understand, return the device name given a mac. Many implementations of this however don't use MAC-Addresses and call this argument device, thus triggering the pylint warning. I don't know what to do about this.

inconsistent-return-statements

Makes sure all return statements in a function either return something (e.g. None) or nothing. For example:

def foo():
    if bar():
        return 42
    else:
        return

would trigger this check. However, as described in pylint-dev/pylint#1782, this currently doesn't handle raise - and this erroneous flag is quite common in Home Assistant. Next, I believe correcting this would best be done together with static type checking updates.

All Changes

  • len-as-condition
  • no-else-return
  • bad-whitespace (see comment)
  • too-many-nested-blocks
  • logging-not-lazy
  • stop-iteration-return (locally disabled due to bug)
  • useless-super-delegation
  • trailing-comma-tuple (see commit message)
  • redefine-argument-from-local
  • consider-using-enumerate (see the one case, the length is already checked before, so that works)
  • wrong-import-order (tried to be as consistent as possible)
  • arguments-differ (device_tracker)
  • no-member
  • signatures-differ

Checklist:

  • The code change is tested and works locally.
  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

Comment thread homeassistant/components/isy994.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (80 > 79 characters)

Comment thread homeassistant/components/plant.py Outdated
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.

I believe this should be correct...

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.

yes

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.

TEMP_CELSIUS is a constant. However appending constants is essentially the same as appending variables in terms of speed (*except for potential compiler optimizations, though the speed impact is negligible)

@MartinHjelmare
Copy link
Copy Markdown
Member

The latest pylint version is 1.8.2. Any reason not updating to that instead of 1.8.1?

@OttoWinter
Copy link
Copy Markdown
Member Author

OttoWinter commented Feb 10, 2018

Oh sorry, I wrote 1.8.1 in the description, but I'm running 1.8.2 locally - will update the description. Running with 1.8.1 and 1.8.2 seems to give the same results though.

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 10, 2018

Fine with for device tracker to rename the official argument to device instead of mac

Comment thread pylintrc Outdated
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.

That's too bad :(

@OttoWinter
Copy link
Copy Markdown
Member Author

What about bad-whitespace? With the fix, pylint 1.6.5 now (incorrectly) complains itself about a bad-whitespace. So should we either

  • Revert that commit and re-apply it later when pylint is actually upgraded
  • or disable bad-whitespace by default (potentially a bad idea)?

Additionally, there's another issue that I forgot to mention: Some new checks in pylint 1.8.2, namely c-extension-no-member, stop-iteration-return and useless-super-delegation, don't exist in 1.6.5 - so 1.6.5 complains about a bad-option-value. I'm assuming it's okay to just ignore that locally.

@MartinHjelmare
Copy link
Copy Markdown
Member

Are there too many changes needed to also upgrade pylint to 1.8.2 in this PR?

@OttoWinter
Copy link
Copy Markdown
Member Author

It would work under 1.8.2, but I tried following the advice in this comment

@MartinHjelmare
Copy link
Copy Markdown
Member

That comment is about Python versions. I don't see anything stopping us from upgrading to pylint 1.8.2 and run that on the Travis pylint tox job.

@OttoWinter OttoWinter changed the title [WIP] Preparations for pylint upgrade Upgrade pylint to 1.8.2 Feb 11, 2018
@balloob balloob merged commit 678f284 into home-assistant:dev Feb 11, 2018
@OttoWinter OttoWinter mentioned this pull request Feb 14, 2018
2 tasks
@balloob balloob mentioned this pull request Feb 22, 2018
@OttoWinter OttoWinter deleted the upgrade-pylint branch March 13, 2018 20:19
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants