Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code should follow the python style guide #216

Closed
GoogleCodeExporter opened this issue Apr 4, 2015 · 7 comments
Closed

Code should follow the python style guide #216

GoogleCodeExporter opened this issue Apr 4, 2015 · 7 comments
Assignees
Milestone

Comments

@GoogleCodeExporter
Copy link

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

To enhance code readability (and maintenance ?), the code should follow the style guide.

Original issue reported on code.google.com by [email protected] on 13 Feb 2015 at 9:49

@GoogleCodeExporter
Copy link
Author

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
Copy link
Author

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 [email protected] on 14 Feb 2015 at 5:52

@GoogleCodeExporter
Copy link
Author

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 [email protected] on 15 Feb 2015 at 5:56

  • Changed state: Fixed

@GoogleCodeExporter
Copy link
Author

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
Copy link
Author

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

  • Changed state: Started

@GoogleCodeExporter
Copy link
Author

OK,

I am going to correct the encoding specification immediately.

Séb


Original comment by [email protected] on 17 Feb 2015 at 7:31

@GoogleCodeExporter
Copy link
Author

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 [email protected] on 17 Feb 2015 at 8:04

richardjgowers added a commit that referenced this issue Apr 9, 2015
Changed Universe._psf to Universe._topology

topology.base.TopologyReader now has keyword 'universe' which
gets saved as self._u.  This is then passed to Atoms on creation.

Atoms can now be passed their Universe as kwarg.

All '_things' renamed to 'things' in Universe._topology.

Renamed __is_guessed to _is_guessed in Bond (death to double
underscores)

Added absolute_import from future to all topology modules (Issues #132 #216)

Updated tests to reflect new names
@orbeckst orbeckst added this to the 1.0 milestone Apr 9, 2015
@orbeckst orbeckst mentioned this issue Aug 28, 2015
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants