djay0529 / mdanalysis

Automatically exported from code.google.com/p/mdanalysis
0 stars 0 forks source link

Code should follow the python style guide #216

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
While our code is mostly clean and well written, most of it does not (fully) 
follow the recommendations from the Python style guide (aka PEP8 - 
https://www.python.org/dev/peps/pep-0008/).
To enhance code readability (and maintenance ?), the code should follow the 
style guide.

Original issue reported on code.google.com by sebastie...@gmail.com on 13 Feb 2015 at 9:49

GoogleCodeExporter commented 9 years ago
pep8 --quiet --statistic AtomGroup.py 
AtomGroup.py
1       E126 continuation line over-indented for hanging indent
3       E127 continuation line over-indented for visual indent
1       E128 continuation line under-indented for visual indent
1       E221 multiple spaces before operator
5       E225 missing whitespace around operator
66      E231 missing whitespace after ','
6       E261 at least two spaces before inline comment
2       E262 inline comment should start with '# '
18      E265 block comment should start with '# '
3       E301 expected 1 blank line, found 0
5       E302 expected 2 blank lines, found 1
4       E303 too many blank lines (2)
1       E401 multiple imports on one line
277     E501 line too long (84 > 79 characters)
1       E502 the backslash is redundant between brackets
4       E701 multiple statements on one line (colon)
9       E711 comparison to None should be 'if cond is None:'
3       E713 test for membership should be 'not in'
15      W291 trailing whitespace
20      W293 blank line contains whitespace

Looks like most the problems are overly long doc/comment lines, how strict do 
we want to be about this?

https://pypi.python.org/pypi/autopep8/

I just tried using this on core/AtomGroup and it worked quite well, especially 
for dealing with whitespace.  I think a mixture of this then checking the 
changes it's made is the easiest solution.

Original comment by richardjgowers on 13 Feb 2015 at 12:01

GoogleCodeExporter commented 9 years ago
I think that for some larger Python projects the usage of automated code repair 
for moderate stylistic violations is not favoured. Instead, the developers tend 
to just fix something if they happen to be working on a module and they notice 
it, rather than sweeping through everything systematically.

I don't have a strong opinion on either side. As you say though, a bit of 
manual checking is probably still needed even with the automatic adjustments.

Original comment by tyler.je.reddy@gmail.com on 14 Feb 2015 at 5:52

GoogleCodeExporter commented 9 years ago
OK, our code is now "PEP8-fied"!
As Tyler feared, manual adjustements was needed and I had to get through all 
the files manually to modify them by hand based on the automated report (from 
Pycharm, the IDE I use). Basically this means:

1. I may have forgotten a few errors here and there
2. I changed only the code, I didn't do (or barely) anything to the docstrings
3. I must admit that I cheated because I used a line length limit of 120 
characters (instead of the classical 79 characters) -> far less "macheted" lines

I also updated the copyright header (and slightly reformatted) to include 2015.

Because of the numbers of changes and decisions I made on my own, I would like 
that you give me your opinion on the new line length limit (120) and if 
everything is OK (I had to play a bit with git rebase).

Cheers,

Séb

Original comment by sebastie...@gmail.com on 15 Feb 2015 at 5:56

GoogleCodeExporter commented 9 years ago
1) New header is fine with me.
2) I don't feel strongly about the line size, so 120 chars is fine with me 
although I'd encourage 79 chars for new code (see the preset for Emacs and vi 
in the header).

Original comment by orbeckst on 16 Feb 2015 at 7:20

GoogleCodeExporter commented 9 years ago
Séb,

Need to reopen the issue because the first header line contains an error in the 
Emacs mode line:

Currently:
# -*- Mode: python; tab-width: 4; indent-tabs-mode:nil; coding=utf-8 -*-
# vim: tabstop=4 expandtab shiftwidth=4 softtabstop=4

Should be:
# -*- Mode: python; tab-width: 4; indent-tabs-mode:nil; coding:utf-8 -*-
# vim: tabstop=4 expandtab shiftwidth=4 softtabstop=4 fileencoding=utf-8

The problem is that this will not be recognized as UTF-8 in emacs and any UTF-8 
chars will get automatically garbled. Also, I think the vim line should also 
contain UTF-8, shouldn't it?

It *might* be ok for the Python interpreter (haven't checked the UTF-8 PEP).

Please can you fix this? Sorry, I didn't catch this earlier and just noticed 
when suddenly my ångström symbols" Å" got messed up.

Also, shouldn't we specify a linewidth in the header as well, such as 79? Ie add

# -*- ..... fill-column: 79
# vim: .... textwidth=79

See eg http://docs.python-guide.org/en/latest/dev/env/

Thanks,
Oliver

Original comment by orbeckst on 16 Feb 2015 at 6:53

GoogleCodeExporter commented 9 years ago
OK,

I am going to correct the encoding specification immediately.

Séb

Original comment by sebastie...@gmail.com on 17 Feb 2015 at 7:31

GoogleCodeExporter commented 9 years ago
Mode lines are corrected and I push the corrected version.

Once the line length is set (cf. 
https://groups.google.com/forum/#!topic/mdnalysis-devel/qQqoaoDow2s), I will 
add the corresponding tags the the mode lines, fix the code that contains 
too-long lines and (re)close this issue.

Séb

Original comment by sebastie...@gmail.com on 17 Feb 2015 at 8:04