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?
--
Neil Williams
=============
http://www.linux.codehelp.co.uk/
pgpOhlU3nFeHP.pgp
Description: PGP signature
_______________________________________________ linaro-validation mailing list linaro-validation@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-validation
