Raise strictness of linting

[agjohnson]

Once #1471, #1472, and #1473 are in place and we feel comfortable with the linting process, we should improve linting further by making the ruleset more strict. This can be managed by Prospector and some tuning of pylint rules, but increasing the strictness profile with Prospector. Additionally, we should consider enforcing stricter rules on line length, as discussed in #1424.

[agjohnson]

Blocking for now, revisit this later.

[gregmuellegger]

This is no longer blocked and therefore open to be worked on.

[ericholscher]

I did a test run with medium strictness, and it outputs a lot, but it's
likely worth doing. It complains about missing docstrings & logging a lot
-- which would be good to raise the quality of our code in general, but it
would definitely be a solid chunk of work.

On Thu, Aug 20, 2015 at 3:43 AM, Gregor Müllegger [email protected]
wrote:

This is no longer blocked and therefore open to be worked on.

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

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

[agjohnson]

We are still ignoring the projects.yaml path, so that should be addressed as per #1472 first.

Medium strictness is definitely worth it, pylint gives more useful errors at this level and beyond. We can tune some of the noise if we want.

For instance, one that is used all over the place is the logging arguments error. I do prefer the log.info('Something %s', foo) syntax, but it's so minor we could just ignore that warning.

Either way, we should be breaking this one out like we did the original linting, to avoid code rot.

[agjohnson]

Also, any thoughts on docstrings? I suppose we should be settings good example and actually documenting everything, but there are a lot of cases where docstrings will be just noise. Do we want to say that if construct usage is very obvious, pylint ignore the line? I suppose at that point, a docstring is no more noisy than a line ignoring the docstring.

[agjohnson]

I dropped an example in #1581, I used this file to make projects/ pass linting in #1580

[gregmuellegger]

I'm not a big fan of adding docstrings to every class and method. If it's a very simple and easy implementation, then the names of the arguments and the methodname will speak for itself. Nevertheless it is surely worth adding some docstrings (I had a hardtime getting behind the meaning of all the fields in the Version model at the beginning for instance).

But I think there cannot be a automated rule on what needs to be documented, and what not. It would be more beneficially to review all of the existing code and add the corresponding docstrings where we think they are useful.

[agjohnson]

I definitely agree. If we can be strict about documentation in review, then we don't need to automate this. If we did want to automate it, we might be able to find a regex pattern that reasonably matches what we should be documenting. Or maybe we set the docstring-min-length even higher, to automate only very non-obvious constructs.

It would be a benefit to have a linting requirement here, as I think the amount, quality, and attention to our reference docs is overall rather poor. If not for onboarding, we should at least be setting an example of good documentation practices

[gregmuellegger]

Yes, let's try it. Turning it off will be possible anytime if it turns out to be hindering.

[agjohnson]

+1, I bumped the minimum length to 20 lines, which sounded reasonable, we can see what sort of errors that accumulates.

[agjohnson]

This is now partially in place, as a second pass to linting. Closing this discussion, we can add new issues for the actual cleanup.