Page MenuHomePhabricator

Reduce the number of stylistic checks
Closed, ResolvedPublic

Description

Currently, we have a large number of flake8 checks:

Some of these are useful, because they catch bugs before they happen:

Some others purely force stylistic choices on the developer:

For those stylistic choices, we should reconsider which ones we feel are really worth the time wasted in getting them right. A good stylistic rule has a few properties:

  • has a strongly positive effect on readability
  • is easy to get right
  • are easy to fix when they are not right
  • provides clear error messages when this is not the case (preferrably they say what you need to do instead of just telling you something is wrong)

From this list, I would say:

  • pep8: probably OK
  • pydocstyle: OK for requiring documentation, dubious when it gets to requiring a certain phrasing style
  • openstack: might be OK
  • flake8-comprehensions: OK-ish, but error messages are so-so
  • flake8-import-order: no, no, no and no
  • pep8-naming: OK

So I would suggest to:

  • turn off flake8-import-order
  • turn off parts of pydocstyle

and maybe turn off more later, depending on what are pain points for developers.

Event Timeline

Another annoyance: being forced to add 'documentation' to functions in test suites.

In general, we should keep in mind that requiring documentation pushes towards longer functions (because splitting up a function becomes tedious). At the same time, I think it might be reasonable to have more 'private' (_xyz) functions, and it could also be a push towards that.

Apparently flake8-putty can be used to make sure pydocstyle (and other tests) is more lenient with the test files than otherwise.

flake8-putty is already in use and tox.ini can be used to exclude style tests

Change 386852 had a related patch set uploaded (by Xqt; owner: Xqt):
[pywikibot/core@master] [test] remove flake8-import-order stylistic check

https://gerrit.wikimedia.org/r/386852

Change 386868 had a related patch set uploaded (by Xqt; owner: Xqt):
[pywikibot/core@master] [doc] turn off some stylistic checks of pydocstyle

https://gerrit.wikimedia.org/r/386868

For import order isort can be used instead. That way the linter can also automatically fix the issues so there is no real overhead for the developer. See e.g. https://gerrit.wikimedia.org/r/378741

@Lokal_Profil: Would be an idea but that automatically fix could lead to a different patch than your commit and it may be disarrange the git log entries of the local repository. Maybe I should get some experience with it.

Change 386852 abandoned by Xqt:
[test] remove flake8-import-order stylistic check

https://gerrit.wikimedia.org/r/386852

Change 386868 abandoned by Xqt:
[doc] turn off some stylistic checks of pydocstyle

https://gerrit.wikimedia.org/r/386868

Change 430608 had a related patch set uploaded (by Dalba; owner: Dalba):
[pywikibot/core@master] tox.ini: Enable E402 and disable I201 checks

https://gerrit.wikimedia.org/r/430608

Change 430608 merged by jenkins-bot:
[pywikibot/core@master] tox.ini: Enable E402 and disable I201 checks

https://gerrit.wikimedia.org/r/430608

turn off parts of pydocstyle

@valhallasw, which parts did you have in mind exactly?

I think every method should have a docstring but something like

def __init__(self):
    """Constructor."""
    pass

does not only make a lot of sense, it is even wrong. That method is an initializer. __new__ would be a constructor method.

That method is an initializer. __new__ would be a constructor method.

That's an allocator. Python's __init__ initializer is mostly functionally equivalent to Java/C++'s constructor.

That method is an initializer. __new__ would be a constructor method.

That's an allocator. Python's __init__ initializer is mostly functionally equivalent to Java/C++'s constructor.

Yeah, I wanted to say the same thing. For people like me, who came to Python from Java-based languages __init__ behaves similar to Java's, Dart's etc. constructors

@Lokal_Profil: Could you setup isort for pwb?

Sorry for dropping this ball.

There is no super elegant way of doing this it seems. We are using it for the WLM bot though, see the tox.ini and the setup.cfg implementation. If that works elegance wise then it should be easy enough to set up locally.

Change 459499 had a related patch set uploaded (by Xqt; owner: Xqt):
[pywikibot/core@master] [check] Don't care about import order any longer; use isort instead

https://gerrit.wikimedia.org/r/459499

Change 459519 had a related patch set uploaded (by Xqt; owner: Xqt):
[pywikibot/core@master] [PEP8] Import sorting order using isort for pywikibot folder

https://gerrit.wikimedia.org/r/459519

Change 459502 had a related patch set uploaded (by Xqt; owner: Xqt):
[pywikibot/core@master] [PEP8] Import sorting order using isort for main folder

https://gerrit.wikimedia.org/r/459502

Change 459503 had a related patch set uploaded (by Xqt; owner: Xqt):
[pywikibot/core@master] [PEP8] Import sorting order using isort for docs folder

https://gerrit.wikimedia.org/r/459503

Change 459522 had a related patch set uploaded (by Xqt; owner: Xqt):
[pywikibot/core@master] [PEP8] Import sorting order using isort for family files

https://gerrit.wikimedia.org/r/459522

Change 459519 abandoned by Xqt:
[PEP8] Import sorting order using isort for pywikibot folder

https://gerrit.wikimedia.org/r/459519

Change 459522 abandoned by Xqt:
[PEP8] Import sorting order using isort for family files

https://gerrit.wikimedia.org/r/459522

Change 459503 abandoned by Xqt:
[PEP8] Import sorting order using isort for docs folder

https://gerrit.wikimedia.org/r/459503

Change 459502 abandoned by Xqt:
[PEP8] Import sorting order using isort for main folder

https://gerrit.wikimedia.org/r/459502

Change 459499 abandoned by Xqt:
[check] Don't care about import order any longer; use isort instead

https://gerrit.wikimedia.org/r/459499

Xqt triaged this task as Lowest priority.Apr 9 2019, 1:02 PM
Xqt edited projects, added Pywikibot-tests; removed Patch-For-Review.

Change 684017 had a related patch set uploaded (by Xqt; author: Xqt):

[pywikibot/core@master] [tests] remove isort from tests

https://gerrit.wikimedia.org/r/684017

Change 684017 merged by jenkins-bot:

[pywikibot/core@master] [tests] remove isort from tests

https://gerrit.wikimedia.org/r/684017

Xqt claimed this task.