Skip to content

VCS etiquette

This document lays out some fundamental rules on how we use version control, create/review PRs etc.

Basic philosophy

The idea of our VCS is to collaborate on our source code, and source code is purely a way for humans to communicate their ideas. We therefore aim to enable better collaboration with our actions in the VCS.

For better collaboration, we all work with some simple common understandings:

  • PRs must not be set to "ready" before they really are. In particular, major AI review findings should be addressed and CI pipelines must be green.
  • The author of a PR owns it and strives to land it quickly. To this end, the PR must visible to potential reviewers (i.e. it should be posted in the relevant Slack channel).
  • The reviewers do the PR author a favour. The author therefore tries to make reviewing as easy as possible.
  • Shorter PRs are easier to review, and the reviews of shorter PRs are more helpful and trustworthy.
  • If it is clear who will review a PR, it should be created with the specific reviewer in mind. I.e. if it's clear in what form they prefer PRs we should try to stick to that. If uncertain, they should be consulted.
  • If a PR involves significant functional changes the DRI must be informed (via Slack). They do not necessarily need to review but they need to be aware.

Following out of these common understandings, we decided to split bigger PRs into a PR stack.

Standard workflow

GitHub provides few mechanisms for enforcing sensible PR workflows. These ground rules make the review process easier for everybody.

Most of these are soft. You can merge without following them, but do follow them unless you have a specific reason not to.

Three are not soft. Do not mark a pull request ready with failing checks or unaddressed significant findings. Do not approve while checks are running. Nobody merges their own pull request without an approving review from someone else. Those are MERGE-2.

Creating a PR

  • Prefer a single, self-contained commit per PR. (Avoid mixing unrelated changes.)

Tip

For simplifying the upcoming review process, considers creating stacked PRs with stack-pr.

  • Open the PR as a draft initially.

  • Fix any failing automated checks by amending commits and force-pushing.

  • Once all checks pass, mark the PR ready for review and find one or more reviewers. Do not assume a team reviewer is aware of your PR. Post a review request in the corresponding PR review Slack channel and if necessary, ping prospective reviewers privately if you're not getting a prompt review.

Reviewing a PR

  • Keep your comments concise.

  • If you are envisioning a concrete fix, add an inline suggestion. For bigger suggestions (significant refactors, multi-file changes etc.) it can be helpful to send the author a proposed patch.

  • Prefix comments with "nit:" if they're just a minor suggestion not affecting functionality or maintainability. Try not to go overboard with these.

  • Submit all your comments at once (if at all possible) instead of bit by bit so the author knows when you're done reviewing. There is one exception. If you notice a major issue that makes continuing on the PR pointless, then leave a few comments and "request changes". See also the next point.

  • When submitting comments, select "request changes" if the PR cannot be submitted as-is for any reason. That means any comment of yours is more than a question or a nitpick. If the author and a reviewer disagree, or the reviewers disagree among themselves, significantly about whether a PR can be merged, then settle it on Slack or in a meeting. If agreement is not possible the maintainer(s) of whatever code is being modified have the final say.

  • Resolve your own comment threads once the author has sufficiently addressed them.

  • Do not hit approve if any automated checks are still running.

Responding to a PR review

  • Wait until every reviewer you've assigned has submitted a review before making any changes to your PR. In particular, do not make changes to your PR until a first review has come in, this can lead to confusion. Instead, if you notice you've made a mistake, leave a self-review (to potentially save other reviewers work). Then address the problems you've discovered together with the comments made by others.

  • Add follow-up changes by ammending and force-pushing. Avoid rebasing unless necessary as this can confuse GitHub.

  • Respond to all review comments at once (i.e. also via the "Review Changes" button) if possible.

  • Respond to every comment you've addressed (can be as simple as adding a "done" or "fixed" comment but only if its obvious what you've changed).

  • Do not resolve threads yourself. The person who started a thread should be the one to resolve it, leave a comment if they forget. An exception is resolving a reviewers open threads after they've left an approval, you may do that yourself.

  • When you are satisfied with your changes and have responded to the comments, request a re-review with the button in the web interface. Optionally ping the reviewers on Slack again.