Skip to content

Git style requirements

Sebastiaan Huber edited this page Dec 9, 2019 · 2 revisions

We expect AiiDA team members to follow the guidelines when creating a pull request.

The guidelines may seem nit-picky at first, but in collaborative projects like AiiDA adhering to such rules becomes increasingly valuable and important over time.

Commits

A commit

  • should ideally address one issue
  • should be a self-contained change to the code base
  • must not lump together unrelated changes

Commentary: While working on a problem, you often find new problems along the way that can make it difficult to isolate certain changes without touching others. That is why git provides powerful tools (e.g. git rebase) to revise your local, potentially dirty commit history after you are done with your changes. If you are not familiar with git rebase, we strongly recommend having a look at one of the many tutorials out there (1,2,3) - it will be a worthwhile investment of your time.

Commit messages

A commit message should look as follows (credits)

Summarize changes in 72 characters or less

More detailed explanatory text, if necessary. Wrap it to 72 characters
(many editors, such as vim, will do this for you). In some contexts, the
first line is treated as the subject of the commit and the rest of the
text as the body. The blank line separating the summary from the body is
critical (unless you omit the body entirely); various tools like `log`,
`shortlog` and `rebase` can get confused if you run the two together.

Explain the problem that this commit is solving. Focus on why you are
making this change as opposed to how (the code explains that).  Are
there side effects or other unintuitive consequences of this change?
Here's the place to explain them to someone else reading your message in
the future. Make sure to include some necessary context for difficult
concepts that might change in the future.

For inline code use backticks like `class SomeClass()` and if you want to
include multi-line code, you can use the markdown concept of indenting it
with four spaces:

    from aiida import example
    example.will_raise()

Further paragraphs come after blank lines.

 * Bullet points are okay, too

 * Typically asterisks are used, preceded by a single space, with blank
   lines in between.

Put references to issues in the description of your pull request, not in
the commit message. Issue numbers are a feature of GitHub, not git, and
the commit log should remain readable in a different environment.

Commentary:

  • Style rules for those who prefer bullet points:
    • Separate subject from body with a blank line
    • Limit the subject line and body to 72 characters
    • Capitalize the subject line
    • Do not end the subject line with a period
    • Use the imperative mood in the subject line
    • Use the body to explain what and why vs. how
  • No need to mention you also added unit tests when adding a new feature. The code diff already makes this clear.

Pull requests

A pull request

  • should be opened only once you consider it ready for review. Each time you push a commit to a branch with an open PR, the reviewers receive an email and notification. It also triggers a CI build, eating up the quota of the aiidateam organization
  • can consist of one or multiple commits. Before you open a PR, make sure to clean up your commit history and create the commits that you think best divide up the total work as outlined above (use git rebase and git commit --amend a lot).

Reviewing pull requests:

  • Do not manually merge the base branch to bring your PR up to date (this will trigger a CI build). The reviewer responsible for merging your PR will do so after the review is completed.
  • If a reviewer requests minor changes, try to use git commit --amend instead of creating new commits, unless you intend the PR to be squashed anyway (see below) in which case it does not matter. While you are at it, rebase your branch before you push, to bring it up to date without triggering an extra CI build.

Merging pull requests: There are three ways of 'merging' pull requests on GitHub:

  • Squash and merge: take all commits, squash them into a single one and put it on top of the base branch.
    • Choose this for pull requests that address a single issue and are well represented by a single commit.
    • Make sure to clean the commit message (title & body)
  • Rebase and merge: take all commits and 'recreate' them on top of the base branch. All commits will be recreated with new hashes.
    • Choose this for pull requests that require more than a single commit.
    • Examples: PRs that contain multiple commits with individually significant changes; PRs that have commits from different authors (squashing commits would remove attribution)
  • Merge with merge commit: put all commits as they are on the base branch, with a merge commit on top
    • Choose for collaborative PRs with many commits. Here, the merge commit provides actual benefits.

Commentary: Ultimately, if you are not sure what to do, please do not merge. @mention someone who does know or come to their office.