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:

  • The author of a PR owns it and strives to land it quickly.
  • 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.

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

Standard Workflow

GitHub unfortunately does not provide a lot of mechanisms for enforcing sensible PR workflows so we lay out some ground rules here that if followed make the review process easier for everybody.

Note that these are "soft" rules i.e. you technically do not have to follow them in order to be able to merge a PR. It is nevertheless strongly recommended you do unless you have a very specific reason not to.

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 (even if people are already included as reviewers via a team don't assume they'll be automatically 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. An exception would be if you notice a major issue that makes continuing on the PR in its current state not sensible, in this case just leave a few comments and "request changes", see also the next point.

  • When submitting comments select "request changes" if you think the PR cannot be submitted as-is for any reason (i.e. any of your comments is not just a question, a nitpick etc.). If there is significant disagreement between author and reviewer(s) or between the reviewers about whether a PR can be merged or not try to come to an agreement on Slack or 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're satisfied with your changes and have submitted your responses to comments, request a re-review by clicking on the corresponding button in the web interface and optionally pinging the reviewer(s) on Slack again.