Add pylint checks to Travis

[moorepants]

We should lint all the Python code with the latest PEP8 at least. I don't see any reason not to follow PEP8 guidelines.

[jcrist]

There are a few checks pylint enforces that may be preferable to turn off. Spaces around operators, for example. Sometimes it's more readable for sympy code not to enforce a*b + c be a * b + c. Other potential ones I can think of right now could be line continuation indents, and spacing requirements between functions.

[moorepants]

Sure, we can do that. But I'm also of the opinion that being nit picky about this doesn't gain much. In general, the pep 8 stuff is a good choice, and makes code consistent. To me it is worth having some of the oddities that pep 8 enforces in place just to stick with the whole standard instead of pieces of it. Since sympy grew organically, now everyone wants their particular preferences when it comes to pep8 and it is virtually impossible to introduce enforced linting at all. I doubt it ever will happen. It's much easier to say: "PyDy source code follows the PEP8." than "PyDy source code follows PEP8 except for ...., ..., ....". We are early enough in development that we can make the first proclamation without too many obstructions.

[jcrist]

Sure, and I agree that for the most part the ones I've listed should be followed. But sometimes I feel that 100% enforcement of those 3 can read to less readable code. The operators rule is great for only a few operations, but for long mathy expressions can make line length longer, and the grouping of operations is harder to follow (IMO). The indentation one also can be a pain. For example:

matrix = Matrix([[1, 0, 0],
                 [0, 1, 0],
                 [0, 0, 1]])

is a lot more readable than doing the mandated 2 indent levels:

matrix = Matrix([[1, 0, 0],
        [0, 1, 0],
        [0, 0, 1]])

SymPy uses some magic that makes enforcing other ones hard. For example, some of the code needs to distinguish between None and False, or True and the Sympy singleton true. Thus, some code has to be written as if expr is True:, which pylint thinks should instead be if expr:.

I'm not opposed to enforcing code quality, but some rules should allow exceptions when the code would be more readable without them.

[moorepants]

I agree that some of PEP 8's rules aren't ideal. But I think we can come up with many more things that we don't like and if everyone chimes in, each person will have a different set of complaints about PEP8. I'm simply for choosing a standard and dealing with the oddities as opposed to creating our own standard to maintain.

If you can list the exact things to turn off in the linters, then we can implement that as "our standard".

BTW, my linting setup (standard install) does not complain about your first matrix example. I've never seen an error about standard 2 indents. I think brace alignment is valid.

Also note that PEP8 does not suggest spaces around all operators:

https://www.python.org/dev/peps/pep-0008/#other-recommendations

I also don't like the spacing between functions rule either. It isn't good for all cases. But I just do it because the linter doesn't complain. It's not that big of deal.

[jcrist]

I suppose my experience has been with flake8 (which does complain about those things), not pylint. If the examples I gave don't error out, then that's fine.

[moorepants]

Interesting, I wonder why all the linters are not the same.

[moorepants]

I can also see how enforcing linting on Travis will create problems when people don't have the same linter installed or don't have the same settings. This could just create a headache. Maybe we just need the "please lint your code" in the checklist idea and be happy if everyone runs some version of linting locally.

[jcrist]

We could just do very limited linting, and recommend people to lint their code locally. Things that should mandatory:

• No whitespace at end of lines
• No extra lines at bottom
• Spaces, not tabs
• Possibly unused import checks (flake8 finds these, not sure about pylint)

[moorepants]

http://stackoverflow.com/questions/1428872/pylint-pychecker-or-pyflakes

Maybe we just want the PEP8 checker and none of the other linting.

[moorepants]

I like a simplified linting idea for Travis.

[moorepants]

I never realized there are so many different flavors of linting for Python...aaaah!!

[mertemba]

Hi, is this issue suited for a GSoC patch?
What is needed to fix this issue, extend the .travis.yml file to install pep8 and run it before build? Or somehow upload the result to another server?
What about using flake8 instead of pep8, as it combines pep8, McCabe and PyFlakes?

[moorepants]

This issue has not been fixed. We'd like to pick one of the linters, probably just pep8 because I don't think we want to enforce other things. Note Jim's list of the basics that we'd like to check.

This PR will also involve fixing those errors in the existing code to get the checks to pass.

[moorepants]

Yes, this will suite for a GSoC patch.

[mertemba]

Ok, thanks. Which list do you mean, everything of PEP8 without the 3 things Jim mentioned above?
Which codes should be checked and corrected, just pydy directory or everything (except examples)? On current master I get ~1000 errors and ~100 warnings.

[mertemba]

Should the Travis build fail when pep8 shows errors?

[moorepants]

I think we want to check a minimum of things. Start with Jim's list of 3 or 4 items.

I think we want Travis to fail for just those 3 or 4 items. It should have a message to the user that says, "please run a linter and fix errors, e.g. pylint, flake8, pep8, etc". This will encourage people to run more linters locally than we run on Travis.

All of the Python code in pydy/should be checked. It may also be nice to check for whitespace in the javascript code (the code we write, not the libraries that we include).

I think we should check the examples too.

[mertemba]

Great, thanks. Sorry, I read over Jim's list three times. This list should be doable for all directories.
Is it correct to use the Python 2.7 linter or should it use Python 3?

[moorepants]

PyDy current only supports Python 2. So please use the 2.7 linter for now.

[asmeurer]

pep8 doesn't seem to warn about x*y + z for me. Nor does pylint. Note that PEP 8, the document, explicitly allows this now, "If operators with different priorities are used, consider adding whitespace around the operators with the lowest priority(ies)." And it even has an example where x*x + y*y is preferred over x * x + y * y.

I recommend looking at pyflakes first (it only checks actual logical errors, like undefined variables). You can use flake8, which combines pyflakes and pep8, if you need to ignore an error or turn a test off. pep8 does style checking. You'll probably end up turning some tests off if you use it (anyway I find its indentation rules to be annoying myself).

[moorepants]

Thanks for the tips @asmeurer. I think our goal here should be to check some minimum set of things on Travis. If Travis checks some basic errors, then it will encourage people to run some kind of linter locally, in which case they'll probably clean up their code further than what Travis suggests. So instead of this being a hardline to cross to get code submitted, it just nudges people in the right direction.

[mertemba]

Thank you both for your comments.
Flake8 detects a lot of unused imports in __init__.py files, which seem to be there on purpose. What to do with them, should flake8 ignore them?

[mertemba]

Ok, I created a pull request, please have a look at it.
The current lint checking just ignores __init__.py files, as they seem to contain unused imports on purpose.

[mertemba]

Do you know what the booger directory is during build and why it is created?
See https://travis-ci.org/pydy/pydy/jobs/55943689#L646.

[moorepants]

Haha, yes that's a temporary directory created during the code generation tests.

[mertemba]

Ok, apparently the temporarily generated code is not PEP 8 compliant. :)
I will just rename it to .py.bak during the linting, if this is acceptable.

[moorepants]

The rename sounds fine. It is actually purposely not pep8 compliant to make the code generation easier and cleaner.

[moorepants]

Can you have travis remove the booger dir before linting instead?

[moorepants]

Or ignore it?

[mertemba]

Is it ok to delete booger/ completely? Maybe it is cleaner to change the . (linting everything) to specific directories (esp. for JavaScript files, as you mentioned in your comments).
Is it ok to check just the Python files in bin/, docs/, examples/ and pydy/, and Javascript files only in pydy/viz/static/js/dyviz?

[mertemba]

Sorry for having that many false positives, these were all found by flake8. I should have it ignore F403, so that from ... import * never throws an error.

[mertemba]

Should I add new comments, partially reverting the previous ones, or make a new branch?

[moorepants]

Is it ok to delete booger/ completely?

I think so, but I'd have to look closer.

Maybe it is cleaner to change the . (linting everything) to specific directories (esp. for JavaScript files, as you mentioned in your comments).
Is it ok to check just the Python files in bin/, docs/, examples/ and pydy/, and Javascript files only in pydy/viz/static/js/dyviz?

I think it is better to exclude than include because we'll never remember to include things that we add in the future.

[moorepants]

Should I add new comments, partially reverting the previous ones, or make a new branch?

Just work on this same branch and keep pushing commits to it. The PR will update accordingly.

[mertemba]

Ok, thanks. I just noticed I skipped over the --exclude option of flake8.

[moorepants]

Another thought on this is to just use a service like: https://landscape.io/

I've never tried it or know if it costs anything.

[mertemba]

Ok, landscape.io looks like a good general option for code checking, maybe more semantically than syntactically, compared to linters.
For open source projects it is free. I will have a more detaileđ look at it when returning home again.

[mertemba]

Thanks for merging. I am currently trying out https://landscape.io/, but the check seems to time out after 30 minutes. Apparently this service is relatively new, so maybe they will fix this soon.

BTW, how did you make those checkboxes and the 7 of 7 tasks complete bar, is this supported by GitHub or a custom/external plugin?

[moorepants]

Task lists are supported by Github: https://github.com/blog/1375%0A-task-lists-in-gfm-issues-pulls-comments

See here for our standard check lists for PRs: https://github.com/pydy/pydy/blob/master/CONTRIBUTING.rst

[mertemba]

Great, thanks!
landscape.io checks still time out. Let's see if this will be fixed.