On 16 July 2013 12:02, Neil Williams <codeh...@debian.org> wrote:
> On Tue, 16 Jul 2013 10:13:50 +0200
> Milo Casagrande <milo.casagra...@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).
--
James Tunnicliffe
_______________________________________________
linaro-validation mailing list
linaro-validation@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-validation
