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).