style guide

With an active and diverse developer community we need to spell out more clearly what we want the code to look like, i.e. we need a Style Guide.

Related questions have come up recently in #392 and #393 but there are also the recurring discussions about length of lines etc., in particular see #216 !

A very basic guideline consists of the editor settings:

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

only use spaces for indentation
one level of indentation equals 4 spaces

Generally, PEP 8 is a good starting point (see #216 )

Either edit this post or add a comment and someone will add an item to the list below.

Questions to settle

Code

line length (80 79 chars)
indentation (spaces, 4 per level)
imports (absolute, aliasing eg import numpy as np import numpy as np #392)
capitalization (underscore_methods, CapitalClasses, etc as per pep8)
camelCase or underscores (underscores, see goodBye camelCase #389)
~~add style checker to travis~~ (nope, but encourage style helpers)
Where does Cython code live Source file locations #444 ?

Testing

how to write tests (see Unit Tests: developer guide and relative imports and nosetests #189)

Documentation

git commit messages (subject line, empty line, body, see e.g. a note about git commit messages)
Move docstrings to numpy style
finally immortalise as wiki page Style Guide

I think last time we looked at line length, 80 was far too small, so 100/120 is probably better. I'm also happy to be very flexible about this (for readability).

We looked at automated systems (autopep8, yapf) for style and they kind of work, but still require a manual pass afterwards.

One option is we could add a pep8 check of some sort to travis, This would be a little annoying, but would make sure that the code doesn't gradually deteriorate after the #216 tidy up.

@richardjgowers

@richardjgowers 100/120 is too long. But just write as long as we can, and like you said, to use autopep8 or yapf to format. Problem solved.

I don't think adding pep8 check is a good idea. Which program you're going to use? I used programs to check pandas' code and there are some failed sections.

I think using a pep8 checker is a good idea. I started using one for MDSynthesis a while back, and though I felt it was a bit too strict at first (and painful to fix all the violations), I now consider it invaluable for maintaining consistent and readable style throughout the library. I think I would enjoy diving into MDAnalysis code more if something like this was used to enforce pep8 conventions. Silly inconsistencies in style are distracting, and make it hard for others to contribute.

Also, 80 characters is just fine. I don't think there's really a compelling argument for going over that, since it just makes it harder to review code, either locally or on Github. I do not consider 100 or 120 characters acceptable for readability.

80 column limit

I found that using 80 columns is the best compromise when I work on my laptop and want to have
two documents side by side (14" with FullHD resolution). I think the maximal line length I can then watch without linewrapping is something between 90 and 100. This is also true for people using tiling window managers (or editors emulating them like emacs/vim). In my personal projects I use 80 characters and above that limit when it helps readability. (As a side effect I use short descriptive names and avoid deep nesting of loops by moving things into reusable functions)

Code linters

YAPF seems to me to be better then autopep8 at a first glance, I think I have some time this week to play with the settings some more. But I guess it won't produce perfect results since code-linting in python is just really hard. I haven't defaulted to using code linters yet for my python projects but in my C++ projects I have emacs configured to run clang-format on every save (This is a huge help since I completely stopped to think about getting the formating right myself). Since not everybody is using emacs a simple solution might be an installable git-hook that applies the code-linter to every added file.

I would also be against a pep8 checker on travis. I see pep8 as a guideline I should mostly follow
but sometimes it just isn't a good fit for the code.

btw great talk about the topic
https://www.youtube.com/watch?v=wf-BqAjZb8M

Syntax Checkers

Most modern IDE's for python have warning about PEP8 violations enabled by default (see pycharm) and for other editors there are good plugins. For emacs I use Elpy, Python-mode is similar for vim.

Imports

I'm for using common abbreviations for packages in the python-scientific stack. Most people are
used to them and writing them out is more confusing (at least for me).

I have habit writing free style and then use yapf to reformat. yapf -i my_file.py. :D

and I found putting those lines in the top of the code makes me very distracting.

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

On 1 Sep, 2015, at 13:15, Hai Nguyen wrote:

and I found putting those lines in the top of the code makes me very distracting.

-- Mode: python; tab-width: 4; indent-tabs-mode:nil; coding:utf-8 --

vim: tabstop=4 expandtab shiftwidth=4 softtabstop=4 fileencoding=utf-8

Is it because you're not using a decent editor? ;-)

(These lines are to stay because I am old fashioned and don't want to rely on automated formatting but have my emacs know what the settings are.)

@orbeckst

@orbeckst

I am actually very low tech and old fashioned too, using only vim with very minimal support.
But I bet we can update the default in .vimrc (or .emacs??) too.

this is all in my .vimrc :D

set nocompatible
filetype off

filetype plugin indent on
syntax on

set tabstop=4
set shiftwidth=4
set expandtab
set textwidth=90
syntax off

" autocomplete
imap <Tab> <C-p>
imap <C-l> <C-o>A
imap <C-o> <ESC>
imap <C-k> <C-o>lli

" go to next line in insert mode
imap <C-j> <ESC>o
" the rest of sentence after cursor to the next line
nmap <C-j> i<Enter><ESC>w

set backspace=indent,eol,start

syntax on
colorscheme delek

" adding 4 spaces in normal mode
nmap <C-k> i<space><space><space><space><esc>

" pydoc
" Usage: move cursor to python keyword in normal mode and SHIFT-K
let g:pydoc_open_cmd = 'vsplit'
let g:pydoc_highlight = 0

On 1 Sep, 2015, at 13:22, Hai Nguyen wrote:

But I bet we can update the default in .vimrc (or .emacs??) too.

I like it publicly stated and semi-enforced whenever one loads the file.

I have another item for the list. commit messages

Having good commit messages makes git blame much nicer to use and finding out the reason for changes is much faster.

I added it to the list.

On 5 Sep, 2015, at 12:00, kain88-de wrote:

I have a mother item for the list. commit messages

Having good commit messages makes git blame much nicer to use and finding out the reason for changes is much faster.

Oliver Beckstein * orbeckst@gmx.net
skype: orbeckst * orbeckst@gmail.com

Test Data and separate package

I like having test data and tests together, instead of relying on an internet connection to pull in data that might or might not be out of date

Inside of the git repository the test and test-data would still be there. So as a developer running the test-suite I wouldn't have to download any files from the Internet. The thing that changes is that we don't over the tests as a package for users anymore. We over that users can download individual files as for the example scripts that we have in the documentation.

PDB, XTC = mda.load_datasets(['PDB', 'XTC'])
u = mda.Universe(PDB, XTC)
# show how a particular function is supposed to be used

So an internet connection would be required to run the examples from a user-perspective. What we do loose with such an approach is that I could verify a package download from PyPi by running the test-suite in the MDAnalysisTests package. None of the other packages I use/know bothers with that, it's just a decision we have to make if we want users to be able to verify somehow that the package they installed works as intended.

Git commit Messages

Actually I think the blog-post from Tim-Pope I linked earlier is a pretty good guideline of what a good commit message is. As I'm teaching colleagues git now I noticed that I can preach to them as much as I want how they should write commit messages it works best if I show them what I mean for a commit of their own. It takes time because I have to do it for each one individually but it is the only thing I found that works. Maybe we could write that a Lab-Book is a good analogy to the git-log.

@richardjgowers

I updated the Style Guide on the wiki. I think it now contains everything discussed here but perhaps someone else (such as the usual suspects @richardjgowers , @kain88-de , @tylerjereddy ) can do a quick read-through and if need be, add missing parts.

Once this issue is closed, we'll declare the Style Guide to be official. I'll send an email to the dev list that the Style Guide is now the law of the land, and from then on code reviews can fully reference it to ask for amendments in line with the Style Guide.

@kain88-de

Re: @kain88-de 's comment on adding tests back into the package: For right now the Style Guide says that we have the split between package and testsuite; this discussion is really a separate issue (so by all means, open one) and has not been decided so for right now, the status-quo prevails.

Maybe add that we should use the six module for ranges and zip in new code like we do in the tests. So new code would already be Python 3 compatible. We have to add it later anyway so we might just do it now

On 22 Oct, 2015, at 13:59, kain88-de wrote:

Maybe add that we should use the six module for ranges and zip in new code like we do in the tests. So new code would already be Python 3 compatible. We have to add it later anyway so we might just do it now

Good idea, can you add an entry + example, please?

Oliver Beckstein * orbeckst@gmx.net
skype: orbeckst * orbeckst@gmail.com

I'll add the part about python 2 and 3 compatibility.
Otherwise there is the thing about 79 or 80 characters limit left.

Besides of that things look good to me.

On 22 Oct, 2015, at 14:20, kain88-de wrote:

Otherwise there is the thing about 79 or 80 characters limit left.

I vaguely recalled that there was a discussion but didn't find a conclusion. You were one of the proponents of 80 chars; I really have no strong feelings either way and it would be fine with me to diverge from PEP8 in that respect.

I'm also ok with either one. I just have to edit the emacs mode-line to a fill-column of 79 so that emacs does the right thing in MDAnalysis.

Oh and the thing with the test. I think we can reevaluate that once we have restructured the tests, until then it's fine right now.

Let's settle on the canonical 79 chars (if nothing else, removes one paragraph from the style guide because we can say "like PEP8").

Style Guide is up and running and from henceforth the law of the land.

(Need to add the SG as a file CONTRIBUTING.rst or HACKING.rst to the repo... but will raise a separate issue.)

Use contributing.rst that will automatically be picked up by github and shown on PRs and issue reports

On 23 Oct, 2015, at 23:11, kain88-de wrote:

Use contributing.rst that will automatically be picked up by github and shown on PRs and issue reports

Thanks, I created #508.

Navigation Menu

Repository navigation

style guide #404

Questions to settle

Code

Testing

Documentation

80 column limit

Code linters

Syntax Checkers

Imports

-- Mode: python; tab-width: 4; indent-tabs-mode:nil; coding:utf-8 --

vim: tabstop=4 expandtab shiftwidth=4 softtabstop=4 fileencoding=utf-8

25 remaining items

Test Data and separate package

Git commit Messages

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

style guide #404

Description

Questions to settle

Code

Testing

Documentation

Activity

richardjgowers commented on Aug 29, 2015

hainm commented on Aug 29, 2015

dotsdl commented on Aug 29, 2015

kain88-de commented on Sep 1, 2015

80 column limit

Code linters

Syntax Checkers

Imports

hainm commented on Sep 1, 2015

hainm commented on Sep 1, 2015

orbeckst commented on Sep 1, 2015

-- Mode: python; tab-width: 4; indent-tabs-mode:nil; coding:utf-8 --

vim: tabstop=4 expandtab shiftwidth=4 softtabstop=4 fileencoding=utf-8

hainm commented on Sep 1, 2015

orbeckst commented on Sep 1, 2015

kain88-de commented on Sep 5, 2015

orbeckst commented on Sep 5, 2015

25 remaining items

kain88-de commented on Sep 23, 2015

Test Data and separate package

Git commit Messages

orbeckst commented on Oct 22, 2015

orbeckst commented on Oct 22, 2015

kain88-de commented on Oct 22, 2015

orbeckst commented on Oct 22, 2015

kain88-de commented on Oct 22, 2015

orbeckst commented on Oct 22, 2015

kain88-de commented on Oct 22, 2015

kain88-de commented on Oct 22, 2015

orbeckst commented on Oct 24, 2015

orbeckst commented on Oct 24, 2015

kain88-de commented on Oct 24, 2015

orbeckst commented on Oct 26, 2015

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions