Add code linting

[ghost]

Could we re-enable linting in the Travis-CI build? My hope is to avoid my own tendency to introduce poorly written code, while encouraging the community as a whole (which has grown recently) to move the code base towards a cleaner style.

[agjohnson]

Well, the issue here isn't that linting isn't enabled, it's that we don't have an official position on linting to enforce yet. So preceding this will require us to take a stance on what style guides we want in place for python and javascript. This is not a bad thing, and I'm 👍 on standards here. It will depend on what we agree on though.

I'd be happy to enforce a subset of pylint rules, but this would definitely require some cleanup effort that I'm not sure we have the time to focus on currently. Maybe focusing on this incrementally is a half-step to enforcing some linting measures?

[ghost]

So the strong options seem to be pep8, pylint, and flake8.

The later is actually a wrapper around pyflake and pep8.

I filed merge request !1437 as a proposal for getting started with enforcing linting rules. It removes flake8, and incorporates pep8, which seems pretty standard for Python projects. Furthermore, to accommodate the fact that little of this code complies with pep8 I added tox.ini with a list of directories to exclude. pep8 respects the pep8 configuration found in tox.ini.

My idea is that as directories are cleaned up, they could be removed from the exclusion list.

[ghost]

Setting max line length to 120, and ignoring migration directories, almost all remaining errors are E265 block comment should start with '# '.

I could probably get through those pretty quickly.

[agjohnson]

My preference is pylint, as I already use that against code I introduce here, and it provides more than pep8 compliance, but other opinions welcome on this.

cc @ericholscher and @gregmuellegger

[gregmuellegger]

I would be in for using a linter over a code-style only checking. But flake8 works pretty well in my experience.

I have not used pylint so far. I justed tested it out on the code base and it seems as it is reporting lots of false positives like not knowing about Django's reverse related managers: E:694,15: Instance of 'Project' has no 'versions' member (no-member)

So I'm happy with using flake8 as a start, but I'm not keen on the 120 line length. I favor the recommended 80 chars of PEP-8 which gives me the ability to view more files at once in split views in my editor.

However having it to 120 might be a good start just to get going.

[ericholscher]

I am the one guilty of having lines over 80. I guess we should aim for that as a goal ☺️ -- I believe pylint is the old stuff, and flake8 is the newest version that has fanciness, in my most technical understanding :)

Perhaps @sigmavirus24 has some thoughts?

[agjohnson]

I don't believe pylint was superseded by any of the pep8, pyflake, flake8 migrations. Pyflakes has some overlap, but is not as useful in my experience. I'd use flake8 next though, and can always run my linting on top of flake8, that would be just fine.

Pylint does require some agreed upon tuning before it's useable, and would likely be improved by https://github.com/landscapeio/pylint-django to stop various metaclass errors, though I haven't used it. Maybe i'll make a pylint config to check in so we have it.

And I code at 80 characters fairly strictly as well, though I don't have a strong opinion about style as it pertains to others.

[ghost]

pylint includes a sub-set of pep8 rules.

flake8 just runs pep8, pyflakes, and Ned Batchelder's McCabe script.

I personally don't know whether pylint or pyflakes is better. They both look actively maintained. Though, as mentioned earlier, pylint seems to catch quite a bit more, and provides great summaries. I have used pylint in previous projects.

pylint-django looks interesting. Would be worth seeing if that plugin tames pylint a little on a django project.

[sigmavirus24]

So disclaimer, I maintain/develop/whatever you want to call it Flake8, pep8, pyflakes, mccabe, and a bunch of Flake8 extensions (of which we have lots). I personally don't see Flake8 and PyLint as mutually exclusive tools. As @destroyerofbuilds points out, PyLint doesn't do as much as pep8. pyflakes overlaps with PyLint but I don't think it's a subset of PyLint at this point.

tl;dr Why not both?

PyLint is great, but the problem I hear most often from people using it is that they need to manage a rather large config file to get it exactly as they like it. That's something the community will have to decide upon since people's opinions about what to set in those config files often vary wildly.

I'd also like to endorse anything that @landscapeio has because @carlio has worked really hard on that stuff and landscape.io is a great service that I use so I don't have to add PyLint to my gate jobs.

Y'all could start with Flake8 and then organically grow PyLint settings as you find them to be desirable/necessary. You can also set a different maximum line length. I know some people live and die by 80 characters per-line, but projects like urllib3 tend to do just fine in the 90s. The PEP actually recommends line lengths than something in the 90s now. (They might say a hard cap at 99 but if you talk to Raymond Hettinger, he'll tell you anywhere in the 90s is fine as long as the team working on the project agrees upon it.)

Also, the code-quality mailing list is open to everyone who wants to discuss this kind of stuff. PyLint, Flake8, etc. use that as a communal mailing list and its managed by people from PyLint, Flake8, pep8, etc.

[agjohnson]

flake8 definitely seems like the right way forward to start cleaning up our codebase. Pylint is a lot more critical of coding style and there definitely is a curve to getting it tuned enough to be useful and not overbearing. If we migrate to pylint after, that's all fine with me, otherwise I'll continue using both if I find it necessary.

We'll have our work cut out for us, cleaning up years of code. Having strict checking at the beginning might impede those efforts. Incrementally bringing in paths that are passing linting against flake8 is probably the right way to start.

I did just test pylint_django, and it works well, except for string references on ForeignKeys, which is a known issue.

[sigmavirus24]

So, I'll just reiterate, flake8 and pylint aren't mutually exclusive. Several OpenStack projects run both as part of their CI. Every OpenStack project runs Flake8 + OpenStack's Flake8 plugin, hacking. Each project configures Flake8 by using config files included in the repository to ignore rules and such. For example, one project which is slowly moving onto Flake8 has a long list of ignored error codes. Not because they philosophically disagree with those warnings, just because they introduced this long after other problems crept in and they didn't want to fix them all at once.

Further, I'll point out that they use tox to run this so you don't have to have the linters installed globally. You could do this with both pylint and flake8 and run both. Both flake8 and pylint are fast, so performance shouldn't be an issue.

[agjohnson]

So, I'll just reiterate, flake8 and pylint aren't mutually exclusive.

And yes, this is a good point, there isn't any reason to not perform both once we are ready. I'll be using flake8 + pylint in this fashion as we go forward anyways.

In terms of line length error cleanup efforts, which is the majority of flake8 output for us if we disregard migrations, we have 550 lines >80 characters, 300 lines >90 characters, and 160 lines >100 characters.

I'm not into the 120 char length. I'd prefer to address these issues with linting if we take a stance on it, instead of kicking the can down the road. 80 is ideal for me, as i'm tuned to edit multiple files side by side at 80 chars, but again, I don't have a strong opinion here. <100 is fine by me and would be an easy target for cleanup.

[carlio]

Hi all, thanks for the glowing reference @sigmavirus24!

Obviously I endorse using Landscape but I'm clearly biased. Also please wait 24 hours because my workers got banned from the GitHub API and nothing works right now. The diff between code though is designed to help improve from a starting point, because nobody has perfect code at the beginning.

You may also want to consider prospector which runs pylint, pyflakes, pep257 etc. It aims to solve exactly the problem you talked about. It has some default profiles and behaviours (including pylint_django) to try to make it easy to get started with static analysis. They're pretty opinionated, but hopefully it stops you drowning in errors the first time you run it. It's designed to be the gateway drug to these kinds of tools, but also be powerfully configurable.

Let me know if you have any questions :)

[agjohnson]

@carlio I just started playing with this. Honestly, when I glanced at the tool this morning, I originally thought the linting chain didn't need another tool in the progression. But prospector does a great job of wrapping up all the tools and setting sane defaults. Strictness profiles is something I really miss from Perl::Critic.

I have a config now that sets us a low bar for easing back into linting. It sets line length <100, low strictness, disables pep257 -- for now! we should probably be following pep257 :) -- and mccabe, and it disregards some larger chunks of code (projects/, builds/) until we are ready to tackle those. Running against master produces a mere 206 issues.

The future plan would be to bump up strictness, axing the warning that are too cumbersome for our project.

@ericholscher @gregmuellegger Are we comfortable with those parameters for now? In terms of line length maybe we say target 80, but <100 is okay as far as linting is concerned?

[gregmuellegger]

I'm fan of 80 chars but it's a higher priority to get the linting in soonish! So 👍 to the plan you have expressed here.

We should open tickets to keep track of long term code quality goals:

• Follow PEP257
• Use <80 line length
• Document required coding styles and coding guidelines
• .. propably more to come ..
• Setting up JS linting (with eslint)

[ericholscher]

👍 as well. Using computers to make me not lazy == <3

On Mon, Jul 20, 2015 at 2:15 AM, Gregor Müllegger [email protected]
wrote:

I'm fan of 80 chars but it's a higher priority to get the linting in
soonish! So [image: 👍] to the plan you have expressed here.

We should open tickets to keep track of long term code quality goals:

• Follow PEP257
• Use <80 line length
• Document required coding styles and coding guidelines
• .. propably more to come ..
• Setting up JS linting (with eslint)

—
Reply to this email directly or view it on GitHub
#1424 (comment)
.

Eric Holscher
Maker of the internet residing in Portland, Oregon
http://ericholscher.com

[agjohnson]

Okay, addressed a lot of the additional clean up work with some ticket that will follow #1450 and #1471.

Closing discussion here, thanks @sigmavirus24 and @carlio for dropping some knowledge!