Introduce PyLint - Githubissues

torbent commented 7 years ago

Starting to use PyLint at T2F we can slowly introduce it to eTools after.

kasfaw commented 7 years ago

@ewheeler let me know how you would like me to prioritize this

ewheeler commented 7 years ago

@kasfaw we have Engineering task label for this sort of thing, so product can ignore anything with this label

ilcsik commented 7 years ago

I did some research on PyLint vs flake8 as they are both widely used but have different approaches on the detail of reports and default configuration level.

Basically in summary what turned out is that PyLint is has more features, but more verbose output and more strict default config - while flake8 is trying to avoid false positives, and remain less verbose and less strict by default (and it uses pep8 and PyFlakes). The general feel is that PyLint needs some serious configuration (mainly due to the strict default config) but once it's "done" it could be more useful. Flake8 is widely used but PyLint seems more praised when one seeks out for linting tool - so while we cannot go wrong with either, I'd go with PyLint.

Also there are 2 options for making PyLint django friendly and avoid complaints like a Class does not have 'objects' member for e.g. in case of Models. One is pylint_django plugin for pylint, the other is django-lint which is a wrapper for PyLint. Pylint_django is nicely covers the special cases for django code while retains the standard pylint report format - while django-lint kind of wraps pylint and its report and make it less verbose, so that it has a feel like flake8.

I do believe that in case of linting the subtractive configuration way (starting with the strict config and removing irrelevant checks along the way) could be more effective, so I went with PyLint and PyLint-django plugin.

Here's a couple of posts that's useful comparing flake8 and PyLint + django:

http://blog.sideci.com/2015/12/14/style-guide-of-python-and-lint-tool/ http://stackoverflow.com/questions/5611776/what-are-the-comprehensive-lint-checkers-for-python?noredirect=1&lq=1 https://www.blog.pythonlibrary.org/2012/06/12/pylint-analyzing-python-code/ https://blog.landscape.io/using-pylint-on-django-projects-with-pylint-django.html

Also the smallest possible comparison I did is an add() function used form an other module, and also imported a non-existent module:

a.py:

from b import add
from b import sub

print(add(1, 2))

b.py:

def add(x, y):
    return x+y

flake8 output:

$ flake8 .
./a.py:2:1: F401 'b.sub' imported but unused

PyLint output (only the messages, all statistics etc stuff removed):

$ pylint lint/
No config file found, using default configuration
************* Module lint.a
C:  4, 0: Unnecessary parens after 'print' keyword (superfluous-parens)
C:  1, 0: Missing module docstring (missing-docstring)
W:  1, 0: Relative import 'b', should be 'lint.b' (relative-import)
E:  2, 0: No name 'sub' in module 'lint.b' (no-name-in-module)
W:  2, 0: Relative import 'b', should be 'lint.b' (relative-import)
W:  2, 0: Unused sub imported from b (unused-import)
************* Module lint.b
C:  1, 0: Missing module docstring (missing-docstring)
C:  2, 0: Invalid argument name "x" (invalid-name)
C:  2, 0: Invalid argument name "y" (invalid-name)
C:  2, 0: Missing function docstring (missing-docstring)

While flake8 doesn't even warn on invalid import by default for example, PyLint is clearly more strict by default.

I'm experimenting with 'partners' module but it throws hundreds of messages so I'm currently trying to fix pep8 related things and also trying to group which checks could be removed or ignored, so after I have some basic understanding on the different checks I'm going to discuss with you so that we can start fine tuning it.

Another question is the CI integration, I have yet to explore how to configure different checks so that they can break the build when fail.

ewheeler commented 7 years ago

@ilcsik thanks for researching this

we use flake8 on https://github.com/rapidpro/rapidpro so my inclination would be to use the same with etools

rapidpro has a pre-commit hook to check code before committing: https://github.com/rapidpro/rapidpro/blob/master/pre-commit.sh

and is configured for CI integration (which in the case of rapidpro is travis): https://github.com/rapidpro/rapidpro/blob/master/setup.cfg

ilcsik commented 7 years ago

I made a PR here https://github.com/unicef/etools/pull/619

Partners app is now fully pep8/flake8 compilant.

@ewheeler @robertavram Can you please review?

ilcsik commented 7 years ago

Reports app is now flake8 compilant