[bitbake-devel] [PATCH 1/4] bitbake: main: fix bad-witespace pylint warnings

[Christopher Larson]

On Tue, Apr 26, 2016 at 3:44 AM, Richard Purdie <
richard.purdie at linuxfoundation.org> wrote:

> This patchset triggered a lively discussion between a few of us about
> whether we should or shouldn't make whitespace changes to the codebase.
>
> There is one viewpoint which is that we should state we're aiming for
> PEP8 and modify the codebase to match.
>
> Unfortunately there are pieces of API and conventions we have which do
> make sense for us and don't match PEP8. For example the use of "d" as
> our datastore variable (its a short variable name) or the camelcase in
> the datastore methods (getVarFlag) which happens to have worked really
> well for the variable dependency analysis.
>
> Personally, I also really don't like unnecessary changes in the history
> so that git blame and code annotation get distorted to the whitespace
> change commits. I probably spend more time doing that than most.
>
> My general thought is that we should aim for something PEP8 like,
> particularly around argument and keyword spacing. I'd prefer to do this
> as we add new code or modify existing code and I have been doing that
> in my own changes for a while to try and improve consistency.
>
> That said, I personally don't get worked up about line length limits
> and even the short variable names aren't that bad, *if* they're limited
> in scope to a handful of lines or follow a codebase wide convention (fn
> is filename, lf is lockfile, e is exception are the main bitbake ones
> that come to mind).
>


Indeed, and 'd' of course in many functions. I think there are a number of
instances in which short variable names are perfectly valid. List
comprehensions, generator expressions, and personally I think it's still
reasonable to use 'i' when iterating over a range in some cases.


So my feeling is we continue to try and improve the style but not go as
> far as large whitespace changes just for the sake of it.
>
> There are some sections which might be worth considering such as Ed's
> changes here to the continuation and line too long pieces of main.py
> specifically as it is currently a mess. I think the short variable
> change may actually confuse some conventions more than fixing them
> though.
>
> Certainly I don't want to see a ton of "PEP8" style cosmetic patches
> hitting the list.
>


I'd agree with all of this. Personally, I'm a fan of leaving the code
better than you found it, rather than doing blanket fixes (admittedly I've
been guilty of the latter in the past on a number of occasions, though :),
so clean up the areas we're working on for other reasons while we're in the
vicinity.

PEP8 specifically outlines that there are valid reasons to deviate from it.
See the "A Foolish Consistency is the Hobgoblin of Little Minds" section.

Regarding line length: "Some teams strongly prefer a longer line length.
For code maintained exclusively or primarily by a team that can reach
agreement on this issue, it is okay to increase the nominal line length
from 80 to 100 characters (effectively increasing the maximum length to 99
characters), provided that comments and docstrings are still wrapped at 72
characters."

It's also worth noting that a number of the things pylint recommends really
have nothing to do with pep8. I'd advise use of 'pep8' or 'flake8' rather
than pylint if you want to avoid potentially unnecessary changes.

Personally, I like to use vim with the syntastic plugin backed by flake8,
and fix the things it identifies when I'm modifying a piece of code already.
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openembedded.org/pipermail/bitbake-devel/attachments/20160426/723ee647/attachment-0002.html>