[Biopython-dev] Further PEP8 Cleanup

[Peter Cock]

On Mon, Dec 3, 2012 at 12:46 PM, Christian Brueffer
<christian at brueffer.de> wrote:
> Hi,

Hi Christian,

Thanks for all the pull requests sorting out issues like this, in
terms of lines of code you'll probably be one of the top
contributors to the next release ;)  This sort of work isn't as
high profile as new features or bug fixes, but has a more
subtle role in the long term of the project - making our code
easier to follow etc. So we do appreciate these contributions.

> I just submitted pull request #102 which fixes several types of PEP8
> warnings (found using the awesome pep8 tool).

101 not 102? https://github.com/biopython/biopython/pull/101

> Here's what's left after those fixes:
>
> $ pep8 --statistics -qq repos/biopython
> 789     E111 indentation is not a multiple of four

That's nasty - although I think we've got rid of all the tabbed
indentation already which was also very annoying.

> 673     E121 continuation line indentation is not a multiple of four

I suspect many of those are a style judgement and done that
way to line up parentheses etc.

> 693     E122 continuation line missing indentation or outdented
> 171     E123 closing bracket does not match indentation of opening bracket's
> line
> 86      E124 closing bracket does not match visual indentation
> 49      E125 continuation line does not distinguish itself from next logical
> line
> 197     E126 continuation line over-indented for hanging indent
> 575     E127 continuation line over-indented for visual indent
> 1092    E128 continuation line under-indented for visual indent
> 773     E201 whitespace after '('
> 540     E202 whitespace before ')'
> 23543   E203 whitespace before ':'
> 55      E211 whitespace before '('

I'd like to see E201, E202, and E211 fixed (whitespace next to
parentheses).

The count for E203 is surprisingly high - I suspect that
could include some large dictionaries? Note some of the
dictionaries are auto-generated so the code to do that
would also need fixing.

> 180     E221 multiple spaces before operator
> 59      E222 multiple spaces after operator
> 5848    E225 missing whitespace around operator
> 6517    E231 missing whitespace after ','
> 2544    E251 no spaces around keyword / parameter equals
> 644     E261 at least two spaces before inline comment
> 346     E262 inline comment should start with '# '
> 156     E301 expected 1 blank line, found 0
> 1838    E302 expected 2 blank lines, found 1
> 364     E303 too many blank lines (2)
> 15553   E501 line too long (82 > 79 characters)
> 857     E502 the backslash is redundant between brackets

Fixing E502 seems a good idea, I suspect many of these are
purely accidental due to not realising when they are redundant.

> 291     E701 multiple statements on one line (colon)
> 122     E711 comparison to None should be 'if cond is None:'
> 3707    W291 trailing whitespace
> 1913    W293 blank line contains whitespace
>
> I'm not sure where to go from here with regard to what's worth fixing and
> what would be considered repo churn (or gratuitous changes that make
> merging of existing patches harder).
>
> I'd especially like to clean up E301, E302,

E301 and E302 presumable are about the recommended spacing
between function, class and method names? If you want to fix
them next that seems low risk in terms of complicating merges.

> ... E701, E711, W291 and W293.

Did you already fix most of those in today's pull request?
https://github.com/biopython/biopython/pull/101

If there are more cases, then by all means fix them too.

> Other items like E251 are more dubious, as some developers
> seem to prefer the current style.
>
> What do you think?

We have a range of styles in the current code base reflecting
different authors - and also changes in the Python conventions
as some of the code is now over ten years old. And if any of
my personal coding style is flagged, I'm willing to adapt ;)

(e.g. I've learnt not to put a space before if statement colons)

As you point out, the "repo churn" from fixing minor things
like spaces around operators does have a cost in making
merges a little harder. Things like the exception style updates
which you've already fixed (seems I missed some) are more
urgent for Python 3 support, so worth doing anyway.

You've got us a lot closer to PEP8 compliance - do you think
subject to a short white list of known cases (like module
names) where we don't follow PEP8 we could aim to run a
a pep8 tool automatically (e.g. as a unit test, or even a commit
hook)? That is quite appealing as a way to spot any new code
which breaks the style guidelines...

Regards,

Peter