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?
Hi
You should use flake8 for checking PEP8 and a few other things, it's the best tool like that out there.
There are tools that automatically, correctly apply pep8 fixes to any python code so if you want a flag day it's pretty easy to implement. PEP8 has a mode where it checks a diff so you can automatically prevent landing PEP8 regressions.
There are various plugins to use flake8 in most editors.
Thanks ZK
I usually use pep8 and pyflakes directly from the command line. As Zygmut said, pflake8 is a handy tool (if I'm not wrong it runs pep8 and pyflakes).
Before a commit I run the files also under pylint, but it is a manual process since it can report lots of warnings that one can actually ignore.
I personally prefer to stick with distro provided tools, without having to go through pip or easy_install, but I guess this will become a problem since we are all using different distros with their own package versions.
Probably we should suggest a minimum version at least for PEP8 and provide a simple pep8 config file to share (we witnessed difference in PEP8 reports between Ubuntu 12.04, 12.10 and 13.04). For the other tools (pyflakes, pylint...) one should be able to use whatever version they have available: we will still go through code reviews and the errors those tools usually report can also be checked at that time.
I'm more keen on having strict PEP8 checking so that the code will (almost) have the same style, and will be a little bit easier to read. I would stick to the 79 chars line length, plus all the "default" PEP8 settings (multiple spaces, no space between operators...).
Ciao.
-- Milo Casagrande | Automation Engineer Linaro.org <www.linaro.org> │ Open source software for ARM SoCs
Hi Milo
I don't think that there's any sane way to get consistent checks without using flake8 (which indeed uses pep8 and others internally) from pypi. There is just too much variability in the packaged versions out there. It's doable but IMHO it's not worth the hassle. Just put the required version of flake8 in some git repo and have people install that if you want standalone "offline" things to work.
Also, flake8 has quite frequent releases that fix bugs (we've gotten our fair share of bugs fixed). If you ever venture to python3 world you really want to have the latest version of tools around.
Good luck ZK
PS: we keep all non-Ubuntu dependencies in a git repository with a bunch of scripts that ensure we deploy exactly that for development. For production we package them to our custom PPA. https://github.com/checkbox/external-tarballs
On Tue, Jul 16, 2013 at 10:26 AM, Zygmunt Bazyli Krynicki zkrynicki@gmail.com wrote:
Just put the required version of flake8 in some git repo and have people install that if you want standalone "offline" things to work.
I guess we will have to do that, yes. There are people that do not use deb based distros, so the PPA would be handy only for those who do. A git repos with the blessed tools would probably work best.
Ciao.
-- Milo Casagrande | Automation Engineer Linaro.org <www.linaro.org> │ Open source software for ARM SoCs
On Tue, 16 Jul 2013 10:13:50 +0200 Milo Casagrande milo.casagrande@linaro.org wrote:
I usually use pep8 and pyflakes directly from the command line.
What's the general feeling about adding comments which override certain (non-PEP8) checks? PyCharm can inspect code and provide overrides for certain functions, e.g.:
class LavaTestData(object): # noinspection PyDictCreation def __init__(self, test_id='lava'): self.job_status = 'pass' self.metadata = {} self._test_run = {'test_results': [], 'attachments': [], 'tags': []} self._test_run['test_id'] = test_id self._assign_date() self._assign_uuid()
This one is relatively trivial but there are some where the relevant change would be more intrusive.
# Set process id if job-id was passed to dispatcher if self.args.job_id: try: # noinspection PyUnresolvedReferences from setproctitle import getproctitle, setproctitle except ImportError: logging.warning( ("Unable to set import 'setproctitle', " "process name cannot be changed")) else: setproctitle("%s [job: %s]" % ( getproctitle(), self.args.job_id))
The advantage is that the IDE then makes it clearer if PEP8 problems are introduced, the disadvantage is IDE-specific comments. (I'm not sure if other tools will make use of such comments.) The comments can be modified in-place to show what is being overridden in the IDE.
As Zygmut said, pflake8 is a handy tool (if I'm not wrong it runs pep8 I personally prefer to stick with distro provided tools, without having to go through pip or easy_install, but I guess this will become a problem since we are all using different distros with their own package versions.
I'm using flake8 2.0 - it adds a useful feature to skip certain files entirely (e.g. deprecated code like lava_test) which otherwise trips up pep8. (Just add # flake8: noqa as a line near the top of the script.)
One other element which I have so far ignored is this behaviour:
try: data = self.start_testcase(test_case_id) except: logging.exception("start_testcase failed for %s", test_case_id)
This raises a warning (in PyCharm and pylint) about "too broad exception clause" / "No exception type(s) specified".
Maybe we should document which pylint outputs we're happy to ignore.
I'm more keen on having strict PEP8 checking so that the code will (almost) have the same style, and will be a little bit easier to read. I would stick to the 79 chars line length, plus all the "default" PEP8 settings (multiple spaces, no space between operators...).
I'm less keen on the 79 character limit - it's going to need quite a few new sub-routines to reduce the indentation.
I'm using this syntax currently:
~/.local/bin/flake8 -v --ignore E501 --show-source
Packaged: pep8 1.4.5-1 pyflakes 0.7.2-1
I've now got lava-dispatcher to a point where there are no problems reported by pep8 or flake8 (modulo the line length) *and* PyCharm is also happy to show all the files as without warnings.
I'll push the changes selectively, there is a proposed merge for files which only need whitespace changes, I'll push files which also need source code changes next (after testing in the multinode branch).
On 16 July 2013 12:02, Neil Williams codehelp@debian.org wrote:
On Tue, 16 Jul 2013 10:13:50 +0200 Milo Casagrande milo.casagrande@linaro.org wrote:
I usually use pep8 and pyflakes directly from the command line.
What's the general feeling about adding comments which override certain (non-PEP8) checks? PyCharm can inspect code and provide overrides for certain functions, e.g.:
class LavaTestData(object): # noinspection PyDictCreation def __init__(self, test_id='lava'): self.job_status = 'pass' self.metadata = {} self._test_run = {'test_results': [], 'attachments': [], 'tags': []} self._test_run['test_id'] = test_id self._assign_date() self._assign_uuid()
This one is relatively trivial but there are some where the relevant change would be more intrusive.
# Set process id if job-id was passed to dispatcher if self.args.job_id: try: # noinspection PyUnresolvedReferences from setproctitle import getproctitle, setproctitle except ImportError: logging.warning( ("Unable to set import 'setproctitle', " "process name cannot be changed")) else: setproctitle("%s [job: %s]" % ( getproctitle(), self.args.job_id))The advantage is that the IDE then makes it clearer if PEP8 problems are introduced, the disadvantage is IDE-specific comments. (I'm not sure if other tools will make use of such comments.) The comments can be modified in-place to show what is being overridden in the IDE.
As long as the comment is portable, not just PyCharm, then I don't have a problem with them in general. I would rather write code that the tools like though.
PEP8 does have a "stop making my code worse" clause (don't follow PEP8 like a robot, the odd justified exception is fine). I suggest we keep that in mind. Uniformity of code makes reading it easier for everyone, but having to make your code less readable to pass a tool is just silly (within reason).
How about; if you have a good reason to not pass a tool (the tool doesn't understand the code well enough to see that it isn't a bug / the style rules make the code less readable) then can we ensure that we put a justification comment as well? If writing the two lines (instruction to tool comment, justification comment) is longer than "fixing" the code, just fix the code.
The guideline from doing code reviews in the past that was given to me was that if you have a problem with style, suggest a functional rewritten bit of code. I actually think that is a useful guideline anyway - if the review isn't constructive, it is just depressing.
As Zygmut said, pflake8 is a handy tool (if I'm not wrong it runs pep8 I personally prefer to stick with distro provided tools, without having to go through pip or easy_install, but I guess this will become a problem since we are all using different distros with their own package versions.
I'm using flake8 2.0 - it adds a useful feature to skip certain files entirely (e.g. deprecated code like lava_test) which otherwise trips up pep8. (Just add # flake8: noqa as a line near the top of the script.)
One other element which I have so far ignored is this behaviour:
try: data = self.start_testcase(test_case_id) except: logging.exception("start_testcase failed for %s", test_case_id)This raises a warning (in PyCharm and pylint) about "too broad exception clause" / "No exception type(s) specified".
In the general case (not the above one) it is too broad and is simple enough to specify 1+ types of exception that you expect to catch. Unexpected exceptions are errors. Of course, I am sure you knew that!
In this specific case I am surprised that any sane unit test system needs that sort of code anyway - surely all the tests are run and any unhandled exceptions are errors anyway?
Maybe we should document which pylint outputs we're happy to ignore.
I'm more keen on having strict PEP8 checking so that the code will (almost) have the same style, and will be a little bit easier to read. I would stick to the 79 chars line length, plus all the "default" PEP8 settings (multiple spaces, no space between operators...).
I'm less keen on the 79 character limit - it's going to need quite a few new sub-routines to reduce the indentation.
In general splitting up code into more functions isn't necessarily a bad thing - it often helps readability. There are also reasonable ways of splitting up lines to avoid hitting 79 characters. While I consider the rule an irritation, having lived with it for a couple of years I find it increasingly easy to just write code that fits without it being ugly or inefficient. Narrow lines are also easier to read for the general populace.
I'm using this syntax currently:
~/.local/bin/flake8 -v --ignore E501 --show-source
Packaged: pep8 1.4.5-1 pyflakes 0.7.2-1
I've now got lava-dispatcher to a point where there are no problems reported by pep8 or flake8 (modulo the line length) *and* PyCharm is also happy to show all the files as without warnings.
That makes me unreasonably happy, but then again, I am a bit odd. Thanks Neil!
I'll push the changes selectively, there is a proposed merge for files which only need whitespace changes, I'll push files which also need source code changes next (after testing in the multinode branch).
On Tue, 16 Jul 2013 12:02:05 +0100 Neil Williams codehelp@debian.org wrote:
On Tue, 16 Jul 2013 10:13:50 +0200 Milo Casagrande milo.casagrande@linaro.org wrote:
I usually use pep8 and pyflakes directly from the command line.
What's the general feeling about adding comments which override certain (non-PEP8) checks? PyCharm can inspect code and provide overrides for certain functions, e.g.:
I'm a bit concerned that much attention is paid dealing with (mostly working around) proprietary, bloated tools. I don't use PyCharm and my preference is to keep not using it, so I'm not sure how much of its adhoc magic I could adhere too. If PyCharm is too smart-ass (usual problem with such kind of tools btw), why not just disable its non-PEP8 particular, or all, warnings?
[]
Hello,
On Mon, 15 Jul 2013 15:17:24 +0100 Neil Williams codehelp@debian.org wrote:
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.
Line length was always my concern during previous codestyle discussion in Infra team. I agree that ~80 char limit feel so last-century. Next terminal line length is 132, and that's what I'd rather use. I cannot say that this proposal was support by everyone. I obviously try to stick with existing codestyle when hacking existing code (including average line length), and writing new small tools you usually don't get into limits, so I didn't hit into issues with line limits lately, though I agree when that issue hits, squeezing the Python code into this (arbitrary by nowadays) limitation is awkward.
So, count my vote for extending line size limit. Or if not, I can keep up with 79 chars too. But I'm sure there's no need to warp LAVA code if it already uses longer lines.
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.
Reading this and other comments, I'm a bit surprised that such issues exist - I don't remember hitting those, even though used python codestyle checks in different projects. After all, PEP8 was created 05-Jul-2001 and didn't change since then (if it did, it would be different PEP), so any variations would be attributed to allowed variations and underspecification in PEP8, subject to tool's user-adjustable configuration (and bugs, yeah, but those would be fixed by now?).
All in all, good practice is to just integrate codestyle checks as part of project configuration, so one can just run "make style" or "./check-style", and get consistent result, with underlying implementation using specific set of options (and specific version of a tool is that's really an issue).
[]
linaro-validation@lists.linaro.org
-
James Tunnicliffe -
Milo Casagrande -
Neil Williams -
Paul Sokolovsky -
Zygmunt Bazyli Krynicki