At LCE13, we proposed to check code against PEP8 (http://cards.linaro.org/browse/LAVA-485) prior to merges and the initial thought was that we could just rely on PyCharm to show the code as "all green" to achieve PEP8 compatibility.
Turns out that PyCharm doesn't restrict itself to PEP8 and can override PEP8 with project-specific settings in some areas (typically line length).
Line length is actually a bit awkward in places and I've been using the default PyCharm length of 120 whilst PEP8 advises 79. To keep the code readable, this could involve a number of new single-use sub-routines to reduce the indenting. Is this worth doing? If not, we can look at a project-specific [pep8] setting in setup.py.
Other tools for PEP8 include pep8 itself, pylint and pychecker.
The command line pep8 has various false positives and false negatives in it's bug history, so we may all have to run the same version and then mandate that version when others test the code too.
pylint disagrees with pep8 and PyCharm on what is an error (e.g. line length 80).
pychecker is more of a source code checker and it tries to import all modules before checking which can lead to errors. e.g. when checking my multinode files, it complained about missing SSL imports when there is no such warning when running the code or in PyCharm.
PyCharm itself includes it's own error checking on top of PEP8 and merges PEP8 with PyCharm's own validation output. e.g. mutable default arguments raise a warning of the same type as a PEP8 warning:
lava_dispatcher/actions/lava_android_test.py def run(self, commands=[], command_file=None, parser=None, timeout=-1): needs to become: def run(self, commands=None, command_file=None, parser=None, timeout=-1): if not commands: commands = []
Arguably, this is probably a correct change, but there are other situations where PyCharm appears to get it wrong:
e.g. in lava_dispatcher/signals/__init__.py (a file which has had lots of changes in MultiNode). PyCharm thinks that this (working) code is an error:
def signal(self, name, params, context=None): self.context = context handler = getattr(self, '_on_' + name, None) if not handler and self._cur_handler: handler = self._cur_handler.custom_signal params = [name] + list(params) if handler: try: handler(*params) except: logging.exception("handling signal %s failed", name)
PyCharm complains of two things here. handler: "'object' object is not callable" and except: "Too broad exception clause". The exception warning is a "weak warning" in PyCharm and wouldn't stop the code getting a "green light". However, the handler code works just fine and it's not clear to me how to change the code to add a __call__ function which will reference the correct handler. Most of the changes I have considered are quite intrusive for this part of the code.
We could document these as "overrides" using a comment but that won't change how PyCharm shows the file, yet this "error" has nothing to do with PEP8 AFAICT.
How intrusive do we want to go for PEP8? PyCharm isn't going to be the PEP8 checker used during packaging or deployment, so what checker are we going to use and how?