◢◢ Pull Requests

Pull Requests are part of every teams workflow and it's easy to overcomplicate the process. Here we will try to define some simple guidelines to keep things moving.

Ship-Show-Ask 🚢

Should code reviews be always blocking and mandatory?

If reviews are for 1) improving quality, and 2) sharing knowledge, do all changes have room for improvement, or bring new knowledge to share? We think not.

To move fast we trust you to do the right thing and take ownership for getting a review when it's necessary.

Here is the framework:

1) 🚢 Ship

Merge your PR without asking for a code review or feedback. This works when you fix unremarkable bugs, add features using established patterns, or do collateral changes like improving docs.

Note: Creating a PR even when it's a simple change helps traceability and viewing the diff easily in GitHub.

If you're going to ship something, please add the merged without review tag to your PR to make filtering/finding easier.

2) 🔍 Show

You create a PR, run CI, merge it without anyone’s approval, and then ask for feedback. This way you deploy quickly but still create space for discussion.

It works when you want to share knowledge but don’t need much feedback, or the feedback may not be blocking.

3) ❓ Ask

You make changes to a branch, open a PR and wait for the review before merging.

Moving Fast

It is the job of the code owner to get reviewers and get the code merged. Gentle nudges within the relevant slack channels, DMs where appropriate are advised. Make sure your PRs are documented and well tested before asking for a review.

Don't make your reviewers do your essential QA!

If you feel like the code your reviewing has not been tested for the basic cases, please DM [[ADP]] so he can review and take action as needed.

Status + Tagging

When creating a PR the aim is to communicate as much information as possible with the smallest amount of friction & work. To do this you should create your PR in one of four states:

1. Draft

Draft PRs indicate that it's still a WIP. Team members are free to take a look and make comments, but not to complete a formal review yet.

When creating a draft PR please make note of what is WIP in the description so your team mates don't make unnecessary comments.

2. PR with no tags

This is the "Please review and merge when ready" state. Make sure to tag the colleagues you need to get a review and get it merged.

3. PR with "do not merge" tag

A PR which is not a Draft but does have the "Do not merge" tag should be reviewed but not merged.

PRs with this state should have good commentary to explain why the PR should be reviewed but not merged.

4. PR with "merged without review" tag

A PR which you intent to merge straight away (under the "Ship" workflow) should include the merged without review tag for visibility.

Reviewing

If you're reviewing a PR feel free to ask questions about logic, decision making and code style.

Nitpicks are important! Import ordering, stale comments and old console logs are annoying, highlight them!

Merging

If it's your PR, it's your job to merge it when you feel it's ready. If you're uncertain, tag a colleague in and get a second opinion.

Etiquette

Each PR should ideally contain a single contextual piece of work.

In the case that there are several small pieces of work that are linked and cannot decoupled a multi-ticket PR is permitted. In these cases please clearly document your reasoning.