875796 - Remove PEP-8 syntax checker from socorro-crashstats build server

Status: RESOLVED WONTFIX

(Socorro :: General, task)

Description

[Brandon Savage [:brandon]]

The purpose of a build system is to determine whether or not the build fails. Failure is defined in many ways, but most broadly means that the code doesn't work.

Linting Python is one thing; checking that it complies with code standards is entirely another. The PEP8 standard itself warns that "A Foolish Consistency is the Hobgoblin of Little Minds" and states that "...most importantly: know when to be inconsistent -- sometimes the style guide just doesn't apply."

Since the spec itself permits violation of the rule for good reason, but the build system does not, we have made PEP-8 a de facto requirement, enforcing upon our code something the spec itself does not permit or require.

Instead, enforcement of the coding standard should be between the reviewer and the developer. Determination of whether or not the rules have been followed, or whether or not even a specific rule should be followed is a human enterprise, not suitable for a machine. 

For these reasons, the PEP8 check should be removed from the socorro-crashstats build system at once.

Comment 1

[K Lars Lohn [:lars] [:klohn]]

hear hear

Comment 2

[Peter Bengtsson [:peterbe]]

The style checking is there to save everyone time. We all agree that it's easier to read and debug code when it looks like you're used to it looking. 

The reason we have it is to avoid wasting a code reviewers time pointing out stuff a computer can do. Equally, thanks to Leeroy the code reviewer now doesn't have to pull down the code and run the tests manually. A computer does that. 

So it's more a matter of saving time. By taking away the doubt and discussion we can speed up code reviews and focus on what matters. 

If you struggle writing as per the (sometimes overly anal) style guide try installing autopep8. Mind you it's not perfect so it might correct things that are irrelevant to your patch. 

Also, to avoid waking jenkins up at all, make sure you have the necessary pre-commit hooks installed.

Comment 3

[Brandon Savage [:brandon]]

What you're saying makes sense, but misses my point.

Since ANY violation of the PEP8 standards could be justifiable, EVERY violation of the PEP8 standards should be afforded the chance to be justified. An automated task can identify the violations. But it can't argue in favor against them. It has only two options: ignore them all or fail them all.

Right now we have it failing to build on every violation. But this violates the spirit of the build system by failing code that might actually work, and violates the spirit of the PEP8 standard by rejecting any opportunity to justify the reasons behind a violation of the coding standard.

I have no objection to a separate process in Github that might check for violations of the PEP8 standard so the reviewer doesn't have to, but since our standard is not to deploy a "broken" build we ought not be failing builds unless they actually fail - from tests, from compilation errors, from syntax mistakes.

Comment 4

[Jannis Leidel [:jezdez]]

This sounds like Jenkins is not setup correctly. I'm not sure if the build should fail hard or not just because one of the PEP8 rules are broken but we could at least use a violations plugin for Jenkins to display those errors and then schedule developer time for code cleanup if necessary. I really doubt we would achieve the same level of consistency if we move code quality control to the code reviews to be honest, at least from my experience that backfired with mixed pull requests/code reviews with even more required effort for the patch developer.

I think Leeroy - which we use to integrate Jenkins build statuses with Github pull request - can't add more information about builds than just SUCCESS or FAILED (CMIIW). That makes deciding if a patch is "done" rather simple: if we want to consider code quality to be a criteria for merging patches then yes, the pull request should stay FAILED. Alternatively we could allow pull requests to be seen as SUCCESS even if there may be code quality errors that need fixing. Personally I find the latter harder to handle since it means the reviewer may forget to look in Jenkins whether a code quality violation has been recorded (assuming we would have configured Jenkins to store the violations with a plugin).

Thirdly, instead of disabling the integration with Jenkins completely (and leaving it to the developers to do repetitive work), I'd like suggest switching to using "flake8", a tool (maintained by Tarek from Servies) that allows per-project settings for which errors should be ignored (http://flake8.readthedocs.org/en/latest/config.html#per-project). Then we could specifically define a list of error codes to be ignored when Jenkins checks the code quality. It also has a nice git pre-commit hook and a neat plugin system for our own conventions if we dare to formulate them (http://flake8.readthedocs.org/en/latest/extensions.html).

Comment 5

[Robert Helmer [:rhelmer]]

I agree with peterbe, I don't see why this is an issue. Automated style check is to avoid wasting everyone's time with syntax-related frippery in the reviews, there's no reason any humans should be wasting time on that (and I guarantee this will creep back into reviews if we don't use an automated style checker, as a reviewee will inevitably forget to run it). Look at some Mozilla reviews like bug 824864 if you want to see an exercise in pain (they working on making syntax check required as part of the build for mozilla-central, but this is much harder in C++ than Python).

As much a I adore the "foolish consistency" quote, I think we need more specifics here to determine if this is causing a problem. Automated style checking seems like a very slight annoyance at worst.

Personally I find code much easier to review when little style issues are not popping out at me.

Brandon, can you point to any specific examples where we purposely want to avoid PEP8, or that it has caused a problem? I'd rather make an exception in the style checker than not run one at all.

Comment 6

[Peter Bengtsson [:peterbe]]

(In reply to Jannis Leidel [:jezdez] from comment #4)

> Thirdly, instead of disabling the integration with Jenkins completely (and
> leaving it to the developers to do repetitive work), I'd like suggest
> switching to using "flake8", a tool (maintained by Tarek from Servies) that
> allows per-project settings for which errors should be ignored
> (http://flake8.readthedocs.org/en/latest/config.html#per-project). Then we
> could specifically define a list of error codes to be ignored when Jenkins
> checks the code quality. It also has a nice git pre-commit hook and a neat
> plugin system for our own conventions if we dare to formulate them
> (http://flake8.readthedocs.org/en/latest/extensions.html).

We already have this working
https://github.com/mozilla/socorro-crashstats/blob/master/.pep8

Comment 7

[Robert Helmer [:rhelmer]]

(In reply to Brandon Savage [:brandon] from comment #3)
> What you're saying makes sense, but misses my point.
> 
> Since ANY violation of the PEP8 standards could be justifiable, EVERY
> violation of the PEP8 standards should be afforded the chance to be
> justified. An automated task can identify the violations. But it can't argue
> in favor against them. It has only two options: ignore them all or fail them
> all.
> 
> Right now we have it failing to build on every violation. But this violates
> the spirit of the build system by failing code that might actually work, and
> violates the spirit of the PEP8 standard by rejecting any opportunity to
> justify the reasons behind a violation of the coding standard.


I don't think this is true - you can propose an exception, see comment 6.

 
> I have no objection to a separate process in Github that might check for
> violations of the PEP8 standard so the reviewer doesn't have to, but since
> our standard is not to deploy a "broken" build we ought not be failing
> builds unless they actually fail - from tests, from compilation errors, from
> syntax mistakes.


I am failing to see why we should not use any automated checking we like as part of the CI process - there's more to code quality than a boolean of "broken" and "works".

Comment 8

[Jannis Leidel [:jezdez]]

(In reply to Peter Bengtsson [:peterbe] from comment #6)
> 
> We already have this working
> https://github.com/mozilla/socorro-crashstats/blob/master/.pep8

Right, that only works with the standalone pep8 project, which albeit it is used by flake8, is just one way for flake8 to determine code quality. The others are PyFlakes and a code complexity checker named McCabe (http://flake8.readthedocs.org/en/2.0/extensions.html#a-real-example-mccabe). Flake8 supports setup.cfg or tox.ini files in projects.

Comment 9

[Robert Helmer [:rhelmer]]

(In reply to Jannis Leidel [:jezdez] from comment #8)
> (In reply to Peter Bengtsson [:peterbe] from comment #6)
> > 
> > We already have this working
> > https://github.com/mozilla/socorro-crashstats/blob/master/.pep8
> 
> Right, that only works with the standalone pep8 project, which albeit it is
> used by flake8, is just one way for flake8 to determine code quality. The
> others are PyFlakes and a code complexity checker named McCabe
> (http://flake8.readthedocs.org/en/2.0/extensions.html#a-real-example-mccabe).
> Flake8 supports setup.cfg or tox.ini files in projects.

Curently we use https://github.com/jbalogh/check which wraps pep8.py and pyflakes (and jshint for JS). I don't think anyone would object to switching to flake8 (although this bug appears to be about something different :))

Comment 10

[Robert Helmer [:rhelmer]]

BTW automated style checking was added in bug 788082.

Comment 11

[[DEACTIVATED] Adrian Gaudebert]

I must say I agree with both opinions here: style checking matters, but sometimes we want to be able to ignore it. For example, when pasting a long link in a comment, we don't really want to break it into several lines. Also, I'm having the case in a branch where I import a module to make it available from there, but without using it, and that causes an error. 

So, here is a proposition: 
* we keep checking and failing at style errors in socorro-crashstats-github. If there is a styling problem, the build fails and github shows it. The reviewer can then go to jenkins and check what the problem is, and decide whether the problem needs solving or is acceptable. That requires the code quality checking to be done last in the build process. 
* we do not check style in socorro-crashstats. We can assume that if the code made it to master, it means developers are OK with it and style doesn't need to be verified again. And I must agree that failing to build a release because of a style issue sounds wrong to me. 

What do you think? Is that possible?

Comment 12

[Brandon Savage [:brandon]]

I think your proposal is quite reasonable, Adrian.

Things like the line split in https://github.com/mozilla/socorro-crashstats/pull/349/files#L1R864 are, in my opinion, less readable than simply leaving it all on one line, violating the 79 character limitation.

Comment 13

[Robert Helmer [:rhelmer]]

(In reply to Brandon Savage [:brandon] from comment #12)
> I think your proposal is quite reasonable, Adrian.
> 
> Things like the line split in
> https://github.com/mozilla/socorro-crashstats/pull/349/files#L1R864 are, in
> my opinion, less readable than simply leaving it all on one line, violating
> the 79 character limitation.

Thank you for providing a concrete example.

This is exactly the kind of thing that could be added to the exceptions file (.pep8), do we need a more complex solution than this?

Comment 14

[Lonnen :lonnen]

I'm hesitant to diverge the build systems between leeroy and non-leeroy builds.

The assertion that we are rigidly inflexible because of our compliance test is simply false. We have the means, through both .pep8 and more complicated bash hacks (like we use to ignore funfactory's pep violation), to ignore just about any exception we think is worth ignoring.