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

Upgrade pylint to 1.7.2 #7108

Closed
wants to merge 2 commits into from
Closed

Upgrade pylint to 1.7.2 #7108

wants to merge 2 commits into from

Conversation

fabaff
Copy link
Member

@fabaff fabaff commented Apr 14, 2017

1.7.2

To-do:

@mention-bot
Copy link

@fabaff, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @sdague and @andrey-git to be potential reviewers.

@andrey-git
Copy link
Contributor

The run emits lots of new errors

@balloob
Copy link
Member

balloob commented Apr 16, 2017

Yeah that happens each time we do this. So let's see what it found and fix it 👍

def async_enable_logging(hass: core.HomeAssistant, verbose: bool=False,
log_rotate_days=None) -> None:
def async_enable_logging(hass: core.HomeAssistant, verbose: bool = False,
log_rotate_days = None) -> None:

Choose a reason for hiding this comment

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

unexpected spaces around keyword / parameter equals

@balloob
Copy link
Member

balloob commented Apr 18, 2017

When we fix issues, we should fix them in dev and just keep rebasing this branch on it. Since they are new warnings, they shouldn't trigger any warnings when fixed in dev.

@fabaff
Copy link
Member Author

fabaff commented Apr 18, 2017

OK.

I was just checking what the implications may be on a couple of files because it will require some time to get rid of all issues.

@fabaff fabaff changed the title Upgrade pylint to 1.7.0 Upgrade pylint to 1.7.1 Apr 18, 2017
@fabaff fabaff mentioned this pull request Apr 18, 2017
2 tasks
@fabaff
Copy link
Member Author

fabaff commented Apr 18, 2017

When we fix issues, we should fix them in dev and just keep rebasing this branch on it. Since they are new warnings, they shouldn't trigger any warnings when fixed in dev.

I think that this approach will not work for a couple of cases.

  • Added new check to detect incorrect usage of len(SEQUENCE) inside test conditions.

Could be fixed in dev.

  • Require one space for annotations with type hints, as per PEP 8.

This was introduced by 1.7.0. Would lead to an issue in dev. no-else-return too. There may be more.

@robbiet480
Copy link
Member

This is gonna be a big changeset. I started it at #7448.

@robbiet480
Copy link
Member

BTW, no-else-return is the worst issue by far but easiest to fix luckily.

@balloob
Copy link
Member

balloob commented May 5, 2017

It looks like PyLint incorrectly detects locks as not being a context manager:

Context manager 'lock' doesn't implement __enter__ and __exit__. (not-context-manager)

We might want to consider disabling that rule for now.

@fabaff fabaff changed the title Upgrade pylint to 1.7.1 Upgrade pylint to 1.7.2 Jun 27, 2017
@balloob
Copy link
Member

balloob commented Jul 5, 2017

I rebased this to see remaining errors.

@balloob balloob mentioned this pull request Jul 5, 2017
2 tasks
@balloob
Copy link
Member

balloob commented Jul 6, 2017

Alright, made progress 👍

For now, I focus on fixing warnings while disabling these extra checks:

  bad-whitespace,
  import-error,
  no-name-in-module,
  too-many-nested-blocks,
  no-member

Which gives us 279 errors left to fix, of which 174 are no-else-return. I am planning on focussing on those first.

@balloob
Copy link
Member

balloob commented Jul 6, 2017

100 issues left. It's all arguments-differ, which looks like it is even raised when the names of the parameters differ. Should be fairly straightforward to align those.

End is near 😉 (for phase 1)

@balloob
Copy link
Member

balloob commented Sep 30, 2017

What should we do with this PR? I gave it a stab and we got pretty close. If no one is going to pick it up, let's close it?

@OttoWinter
Copy link
Member

@balloob If you don't mind, I would be willing to pick this up again. I'm using Python 3.6 on my machine and it's pretty annoying having to constantly switch Python versions just to run pylint... However, I'd then also upgrade pylint to 1.8.1 so that the Unused variable '__class__' bug is fixed as in home-assistant-libs/pychromecast#211.

@balloob
Copy link
Member

balloob commented Feb 9, 2018

would be great. we will still run pylint under our lowest supported version in CI

@OttoWinter OttoWinter mentioned this pull request Feb 10, 2018
16 tasks
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 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.

9 participants