Coding guidelines and code review checklist

This document is for anyone who want to contribute code to the khmer project, and describes our coding standards and code review checklist.

C++ standards

Any feature in C++11 is fine to use. Specifically we support features found in GCC 4.8.2. See https://github.com/dib-lab/khmer/issues/598 for an in-depth discussion.

Coding standards

All plain-text files should have line widths of 80 characters or less unless that is not supported for the particular file format.

For C++, we use Todd Hoff's coding standard, and astyle -A10 / "One True Brace Style" indentation and bracing. Note: @CTB needs Emacs settings that work for this.

Vim users may want to set the ARTISTIC_STYLE_OPTIONS shell variable to "-A10 --max-code-length=80" and run `:%!astyle` to reformat. The four space indentation can be set with:

set expandtab
set shiftwidth=4
set softtabstop=4

For Python, PEP 8 is our standard. The `pep8` and `autopep8` Makefile targets are helpful.

Code, scripts, and documentation must have their spelling checked.

Python-based codespell can be applied to multiple files easily. codespell can be installed via the following:

mkdir ~/bin
git clone git@github.com:lucasdemarchi/codespell.git
cd codespell
make prefix=${HOME} install
export PATH=$PATH:~/bin/

Note, if you want codespell to always be available you will need to add the export line to your ${HOME}.bashrc or equivalent.

To run codespell over only what has been changed on the branch my-branch:

git diff master..my-branch > diff_file
codespell diff_file

To run codespell over a single file:

codespell path/to/file

To make codespell fix the issues it finds automatically:

codespell -w path/to/file

Please note that as codespell works off of a listing of possible misspellings it may not catch all errors. If you find a spelling error that is not caught by codespell feel free to open a pull request at the project page to add it to the dictionary.

Vim users can run:

:setlocal spell spelllang=en_us

Use ]s and [s to navigate between misspellings and z= to suggest a correctly spelled word. zg will add a word as a good word.

GNU aspell can also be used to check the spelling in a single file:

aspell check --mode ccpp $filename

Checklist

Each pull request should be automatically populated with the following checklist:

- [ ] Is it mergeable?
- [ ] `make test` Did it pass the tests?
- [ ] `make clean diff-cover` If it introduces new functionality in
  `scripts/` is it tested?
- [ ] `make format diff_pylint_report cppcheck doc pydocstyle` Is it well
  formatted?
- [ ] Did it change the command-line interface? Only backwards-compatible
  additions are allowed without a major version increment. Changing file
  formats also requires a major version number increment.
- [ ] For substantial changes or changes to the command-line interface, is it
  documented in `CHANGELOG.md`? See [keepachangelog](http://keepachangelog.com/)
  for more details.
- [ ] Was a spellchecker run on the source code and documentation after
  changes were made?
- [ ] Do the changes respect streaming IO? (Are they
  tested for streaming IO?)

Note that after you submit the pull request you can check and uncheck the individual boxes on the formatted comment; no need to put x or y in the middle.

CPython Checklist

Here's a checklist for new CPython types with future-proofing for Python 3:

- [ ] the CPython object name is of the form `khmer_${OBJECTNAME}_Object`
- [ ] Named struct with `PyObject_HEAD` macro
- [ ] `static PyTypeObject khmer_${OBJECTNAME}_Type` with the following
  entries
   - [ ] `PyVarObject_HEAD_INIT(NULL, 0)` as the object init (this includes
     the `ob_size` field).
   - [ ] all fields should have their name in a comment for readability
   - [ ] The `tp_name` filed is a dotted name with both the module name and
     the name of the type within the module. Example: `khmer.ReadAligner`
   - [ ] Deallocator defined and cast to `(destructor)` in tp_dealloc
     - [ ] The object's deallocator must be
       `Py_TYPE(obj)->tp_free((PyObject*)obj);`
   - [ ] Do _not_ define a `tp_getattr`
   - [ ] BONUS: write methods to present the state of the object via
     `tp_str` & `tp_repr`
   - [ ] _Do_ pass in the array of methods in `tp_methods`
   - [ ] _Do_ define a new method in `tp_new`
- [ ] PyMethodDef arrays contain doc strings
   - [ ] Methods are cast to `PyCFunctions`s
- [ ] Type methods use their type Object in the method signature.
- [ ] Type creation method decrements the reference to self
  (`Py_DECREF(self);`) before each error-path exit (`return NULL;`)
- [ ] No factory methods. Example: `khmer_new_readaligner`
- [ ] Type object is passed to `PyType_Ready` and its return code is checked
  in `MOD_INIT()`
- [ ] The reference count for the type object is incremented before adding
  it to the module: `Py_INCREF(&khmer_${OBJECTNAME}_Type);`.
comments powered by Disqus