Pylint and PEP8 presubmit checks fighting

[Ian Clelland]

I'm doing some refactoring inside of the V8 bindings code generation (all python), and it looks like pylint was recently added to the presubmit checks (crrev.com/d198da4). pep8 was already there, so now there are *two* linters expressing their opinions about my coding style.

Mostly, they're in agreement, but I'm finding some cases around multi-line statements where it's very difficult to satisfy both of them.

For example, I'm returning a dictionary from a function, like this:

def constant_filters():

    return {'has_constant_configuration': filter_has_constant_configuration,

            'has_special_getter': filter_has_special_getter,

            'runtime_enabled_constants': filter_runtime_enabled,

            }

If the closing '}' lines up with the opening '{', then pylint is happy, but pep8 complains:

    closing bracket does not match visual indentation  [pep8/E124]

If I line the closing bracket up with the opening quote from the previous line, then pep8 is satisfied, but now pylint says:

    Wrong continued indentation.  [pylint/C0330(bad-continuation)] [5]

Is there a good way to deal with this? I'd rather not have to add "#pylint disable" directives for something like that -- can we just remove pep8 now that pylint is in place? Or make one of them just produce warnings? They're both generating errors now, and git cl upload won't even give me the option to submit anyway.

[Dirk Pranke]

Last I checked, pep8 and pylint checked for and caught different things, and hence there was value in running both. It's possible that that's not really still true (i.e., pylint might be a superset now), and so we should re-check.

Quinten's been fixing a lot of formatting-related things lately, so I'd follow up w/ him.

-- Dirk

[Jeremy Roman]

Does putting the opening brace on its own line help? That's how I'd have done it:

return {

    'a': a,

    'b', b,

[Ian Clelland]

I ended up putting the closing brace on the end of the previous line, which satisfied both. It's possible that either would have worked.

That wasn't the only thing; I spent about 20 minutes cleaning up a bunch of multi-line constructs, looking for the magic formatting that would make both of them happy.

[Dirk Pranke]

I'm curious: there is a format-webkitpy script that does what you'd think it would do. Did you know about it, and did that not produce compliant formatting? Because if it doesn't, that's bad.

-- Dirk

[Ian Clelland]

I did not know about that -- thanks! (Can we hook that up to git cl format?)

(Trying it, it seems that the pep8 import line in autopep8.py is broken; fixing that, it seems to run for a really long time, without writing to any files... oh, it wants a filename, or else it reads from stdin without saying anything...)

I broke my code again (moved the closing brace to its own line) and ran that script on the file. It changed a bunch of other things, but didn't fix that problem. check-webkit-style still complains about the error.

[Dirk Pranke]

I actually thought it was hooked up to git-cl format, but I'm probably misremembering.

-- Dirk

[Quinten Yearsley]

YAPF is hooked up to git cl format, but you have to make sure that you have yapf installed yourself; see bug 571669.

For any particular pylint warnings that are not helpful, we should disable them in webkitpy/pylintrc. If the warnings are a bit too much right now, I'd be happy to revert the CL that enables those warnings for now... I do think there are a lot of warnings, although I believe most warnings are not causing presubmit to fail.

With regards to formatting and indentation of lists/dicts style, I think that the checkers should be OK as long as the code looks like examples in PEP8.