Guidelines for PR writers and reviewers

Every pull request to Catlab should be reviewed by at least one person. Following are some things to check when making a PR yourself or reviewing someone else's PR. The goal of this list is to ensure that the Catlab codebase is robust and maintainable. When any of these guidelines are violated, it should be documented in a comment on the PR page.

Note: This list only includes the mechanical things. When a reviewing a PR you should always use your own judgment in asking questions and making comments about API and algorithm design. That's the hard part!

Style

See the official Julia style guide for general guidelines but note the following additions and exceptions:

• Indent width is 2 spaces
• Try to avoid lines longer than 80 characters. Occasionally it may be convenient to go over slightly, but never do so egregiously.
• Introduce a new struct when many (≥3) functions have overlapping arguments that are common aspects of a shared concept
• Catlab uses modules more often than most Julia packages. While this may be idiosyncratic, it helps keep different components of Catlab isolated, which will be useful in the future when we spin out modules as their own packages.
• Prefer accessor and mutator functions (e.g. dom(f)) over direct manipulation of struct fields or keys (e.g. f.dom), especially when the functions already exist. This convention supports writing generic code and makes it easier to change data structure internals without breaking existing code.
• In long files, using comment headers can improve readability and navigability. Top-level headers and sub-headers should be formatted as:

# Section
#########

# Subsection
#-----------

Tests and code coverage

• Enhancements (new features) must have accompanying unit tests
• Bug fixes should come with unit tests exposing the bug, unless producing a minimal example is unusually difficult
• Do not delete existing unit tests, unless you have a very good reason (e.g., the relevant functionality is being moved to another package), which is documented in the PR
• If you are adding a new module, make sure to add the test module to the test runner (test/runtests.jl or file included therein)
• Code coverage on the Catlab repo exceeds 90% and we try to keep it that way. We do not insist on 100% coverage, which is impractical, nor do we set a hard threshold applicable to all PRs, but generally you should aim for 90%+ code coverage. Any reductions in test coverage should be justified in the PR.

Documentation

• All exported functions, types, and constants must have docstrings
• Docstrings should be written in complete sentences, with correct capitalization and punctuation. Likewise for comments, except for fragmentary end-of-line comments.
• Where possible, provide citations for constructions and algorithms that you implement. This reflects good scholarly values and also aids the understanding of your code by other people.

Version control

• Commit messages should be informative and be written in the complete sentences
• Avoid one-word commit messages like "fix" or "bug". If you need to make very simple fixes on your branch, amend a previous commit and force push.
• Avoid repeatedly merging the master branch back into your PR branch. Instead, rebase off master and force push.

Backwards compatibility

Reflecting Catlab's dual status as a research project and a user-facing library, we want to give ourselves space to experiment while also not annoying our users and each other by needlessly breaking things.

• Like other Julia packages, Catlab aims to follow semantic versioning
• All else equal, it is better to make breaking changes to new APIs, especially very new ones, than old APIs
• If you plan to make major breaking changes, please coordinate with the senior developers to ensure that it makes senses and aligns with the release schedule