-
Notifications
You must be signed in to change notification settings - Fork 362
Contributing
For Python code, we generally follow PEP 8.
We get around Python flexible type system in several ways:
- we try to avoid "magic" (e.g., generating or changing classes on the fly);
- we are fairly verbose with naming, trying to help the reader with following the types;
- we follow our type annotation system for method and function docstrings (planning to switch to PEP 484 when we will remove support for Python 2); see later for the format.
We support both Python 2 and 3, so we use the package six as required. In the medium future we will drop support to Python 2 and remove all temporary measures to support both at the same time. If you write some code that will need to be changed at that time, try to phrase it with six.
We use a custom format for type annotation in method and function docstrings. Here's an example taken from the code:
class Cls(object):
[...]
def example(self, a, b, c=None):
"""Perform an example action, described here in one line.
This is a longer description of what the method does and can
occupy more than one line, each shorter than 80 characters.
a (int): a is a required integer.
b ([{str: int}]): b is a list of dictionaries mapping strings to
integers, and note how the docstring wraps with indent.
c (Submission|None): c is either a Submission (not required to
fully specify, but it could be helpful for symbols that are
not imported) or None.
return ((int, str)): this method returns a tuple containing an
integer and a string.
raise (ValueError): if a is negative.
raise (LookupError): if we could not find something.
"""
Note that
- everything is written with the imperative form;
- there should be an initial stand-alone first line, that can be followed by a longer description, possibly with multiple paragraphs;
- there are blank lines between: the first line, each paragraph of the longer description, the arguments section, the return section, the exceptions section, and at the end;
For very short and simple functions, you can simplify the docstring as needed (but please err on the verbosity side). If a single line, it should look like this:
def sqrt(x):
"""Return the square root of x."""
To contribute, please send an email to [email protected], or ping us on gitter with what you plan to do (unless uncontroversial and/or small), so that we can agree on the best way to implement it.
We appreciate small commits that do one thing, but also that, when possible, each commit doesn't break the master branch. Please use your best judgement for the size of the commit according to these guidelines. If a commit breaks master, we at least require to push together all commits until master is fixed.
We also appreciate a tidy history, so after you write all your code, consider tidying up the commits to reflect what you did at the end, which is usually a simplified version of the process that you followed to reach the final state. Moreover, each commit should not have PEP 8 or pyflakes warnings (see below for how to make sure you don't introduce any).
If your change involves more than one commit, please create a PR for each of them, unless for very small and obvious commits (read: fixing typos, comments, a few obvious lines), or unless some commit breaks master.
During the review, please address all comments by creating one or more 'fixup' commits on top of the branch (no forced push). At the end, either you or one of the owners can squash appropriately the fixups.
When you work on the CMS git repository, please install pep8 and pyflakes and add the following pre-commit hook:
#!/usr/bin/env bash
COPYRIGHT="`date +%Y` `git config user.name`"
FILES=$(git diff --color=never --cached --name-status | awk '$1 $2 { print $2}' | grep --color=never -e \.py$)
if [ -n "$FILES" ]; then
pycodestyle -r $FILES
p8=$?
pyflakes $FILES
pf=$?
if [ "$p8" != "0" -o "$pf" != "0" ]; then
exit 1
fi
MISSCP=0
for i in $FILES; do
git show ":$i" | grep -q "$COPYRIGHT"
CP=$?
if [ "$CP" != 0 ]; then
echo "Missing copyright for file $i."
MISSCP=1
fi
done
if [ "$MISSCP" != "0" ]; then
echo "Some copyright notices are missing for you and for the current year."
echo "If you have done non-trivial contribution in those files, consider"
echo "adding your copyright to it."
echo "Otherwise, use git commit --no-verify to commit."
exit 1
fi
fi
exit 0You just have to write this content in .git/hooks/pre-commit and make it executable (chmod 755 .git/hooks/pre-commit). It does a few sanity checks in the files you're changing and warns you about potential errors and bad styles.
You're usually advised not to ignore these warnings. If you really want to do so (for example, for implementing them in a separate commit), pass the -n option to git commit.
If you'd like to keep different configuration files (config/cms.conf, config/cms.ranking.conf) for each branch (e.g. you need an empty db to test a new feature but don't want to lose your current db), you can use this trick: create multiple configuration files having @branchname as a suffix (e.g config/cms.conf@master, config/cms.conf.ranking@master) and use this post-checkout hook:
#!/bin/sh
current_branch=$(git rev-parse --abbrev-ref HEAD)
cd config
ln -s -f cms.conf@$current_branch cms.conf
ln -s -f cms.ranking.conf@$current_branch cms.ranking.confThis will "change" your CMS configuration each time you do a git checkout branchname. Make sure to do a backup of your config folder before trying this method, because if misused it can overwrite your configuration. Note that the "suffixed" configuration file must exist (you can however adapt the script to, say, use the @master version when there's no @branchname version).
- Create your own GitHub fork of cms-dev/cms.
- Set up Travis to monitor your fork.
- Create a local repository with two remotes: origin (pointing to your fork) and cms (pointing to cms-dev/cms).
- Set up the git hooks as explained above to see style warnings.
- Before starting to write some code, sync master and origin/master to cms/master
- Create a branch for your development and a corresponding remote branch origin/ (on your fork). Here you can rebase, reorganize, force push as you please.
- When you finish coding, rebase and origin/ on top of the current cms/master.
- If has only one commit, create a PR from the branch, and push fixup commits on top as necessary.
- If has many commits, create a new branch _pr pointing to the first commit, push it to origin/_pr and create a PR from that branch, linking origin/ for context; when that PR is pushed, rebase everything on top of the new cms/master and repeat.