Summary

Pull Requests are important at Signal Noise; they’re the main way way we maintain code quality and oversight. This RFC sets out recommendations for how to create a PR and how to review one, as well as the surrounding processes.

Jump to the guide or the reference for this RFC.

Motivation

Well formed, well reviewed Pull Requests are arguablty the most important aspect of coding at Signal Noise.

Because we use the Squash and Merge strategy, the PR is the unit of code that is preserved in the record of the project. Ideally each single ticket is addressed by one Pull Request, leading to one commit in the main branch.

This means the Pull Request the place people in future will scan the code changes made when they’re trying to find a bug, answer a question, or extend the functionality of the project.

Importantly, the PR is also the place the rest of the team review the code going into the codebase, meaning it’s the first and best opportunity to talk about stylistic or functional choices before they become difficult to untangle.

PRs will generally also have a custom CI/CD environment per PR so that anyone on the project can review the proposed changes in their browser without checking out the code.

Guide

The Pull Request is the place to stop and consider the code that’s going into the project in detail. The PR is the unit of code and documentation that will go onto the main branch, and that needs to provide others with the information they need to understand what what your code does, how it does it and why.

If you’re writing the code, you’ll need to [open a PR] and after it’s accepted, [merge it]. If you’re asked to (or if you have responsibility to), you will need to review the PR.

NB: Each Pull Request should cover a single feature or unit of logic.

Open a Pull Request

Many people open a new Pull Request for their branch as soon as they start working on a feature. This means they connect in to all the automated tooling running server-side and have a URL to share with others if they want to discuss their in-progress work. If you’d like to do this, GitHub allows you to mark a PR as Draft when you make it, or to convert an open one to a draft one. Draft PRs cannot be merged.

Please ensure each Pull Request tackles just one unit of functionality.

Either way, at some point you’ll want to open a new PR, if only to get your feature code into the main branch. You can do that in the Github interface (there’s also a custom link in the response to your push on the CLI, if you’re looking for shortcuts).

Take the time to create a nicely formed Pull Request that documents the work you’re doing, and that maintains the code quality in general, by:

Please read the specific guidance on PR contents.

Review the PR

If you’re assigned to a PR please find time to take a look when you get to a natural break in your work, but at least once a day. PRs that stay open for a long time are harder to merge and can block developers or tempt people to use workarounds that can cause problems.

No Pull Request should be merged to main without being approved by the tech lead or lead developer on the project, or at least one other developer if that isn’t possible.

When posting your review think carefully about the type of response you post. The coder who opened the PR has taken time to achieve something and hopefully spent the time on making both their code and the PR clear and understandable. Please be thoughtful about your comments. In particular, do not close the PR without discussion with the person who opened it as this can result in repeated work and is very discouraging. Read more about the responses you should make as a reviewer.

When reviewing code, look out for style, clarity and overall approach.

Merge the PR to main

Please only use the Squash and Merge merge strategy on PRs. This ensures that the main branch’s history remains clean and easy to browse, which in turn lets us commit early and often. Restricting this is configurable in the project’s Github settings page.

Once a PR has been approved, it’s best if the approver merges the PR, just to keep everything ticking over, though note that they will then share credit in the github history. The exceptions are either if the PR is out of date and can’t be merged, or if a comment was made about something that won’t block the merge but that the author ought to read before merging.

After merging, please delete the branch to keep the repo tidy. There’s a setting in the projects Github settings page to automate this. Note that this is another reason to keep PRs small and focused on a specific unit of functionality.

Reference level explanation

This reference section covers:

Pull Request contents

When creating a Pull Request remember that you are creating it for two audiences: the reviewer of your code (who is going to be reviewing against a set of quality-based criteria), and any future developers looking to understand the code and the decisions behind it; each PR becomes a single commit in the main branch once it’s been merged.

To really work well, a PR should only ever focus on one thing: for example to either add or refactor a single “unit” of functionality. Combining things - e.g. refactoring with new functionality, or addressing two different pieces of functionality in the same PR - can lead to sprawling PRs and be awkward to review. It’s good to have everything related to that functionality (documentation, tests etc) all in the same PR.

Specifically, each pull request needs several elements to really work well:

Title

Provide a concise, clear title.

The title should reference the unit of functionality that the PR covers. Remember that the title is going to become the commit message visible in git blame and other tools that people use to discover the decision-making behind the code that weas written, when it’s time to refactor or run updates.

Don’t reference ticket numbers or similar in the title; the PR number will be auto-added by GitHub and the ticket should be referenced in the description.

Stylistically, it’s best if your title is capitalised and uses the imperative mood, with no full stop. The git manpage suggests keeping it under 50 characters as well. Examples and rationale for these choices are nicely explained here

Remember that a PR’s title can be edited at any time until it’s been merged.

Link to any relevant issues that may have more information about the work - with a keyword to close the issue if it’s a GitHub issue.

Put this at the beginning of the description in the PR.

Note that linking to a ticket isn’t something to do in lieu of a description. It’s just polite to make sure everything a reviewer needs to know about a PR is included in the description even if this means copying and pasting from a linked ticket.

Description

Write a clear description of the changes you’ve made, starting with a link to the issue that triggered the PR.

The description is also going to end up as part of the commit message, as well as remaining visible for the duration of the project. It’s important that the description explains the functionality in the code being committed, as well as any design decisions and/or tradeoffs encountered. It’s helpful for the reviewer as well as later developers if you can write the description in terms of what to expect from the functionality, e.g.

When you run this code you should expect to see this thing x which is different from thing y which used to happen.

It’s important to also include any instructions that other developers will need to take to stay up-to-date once this PR has been merged: i.e. installing new requirements or running DB migrations.

If you’re writing a PR and have questions about the code you’ve written (perhaps the approcha you’ve taken for example), it’s better to put these in comments attached to specific chunks of code, since the final PR will read better and be easier to understand if these questions aren’t at the top of the piece.

Remember that a description, like a title, can be edited at any time until it’s been merged.

Code

By the point your PR is ready for review, your code should follow the RFCs and pass any relevant tests. It should also build happily using the CI/CD system in place. If it’s your PR it’s your responsiblity to check that everything looks the way it should on the environment the CI/CD system creates for the PR.

Documentation and tests

A PR should contain any relevant additions or updates to documentation and tests that fit with the code that’s being committed (if any). If the PR is reverted there would ideally not be spillover of tests that expect code from the PR or irrelevant documentation left. Check your README and any other documentation for changes that ought to be included in the PR.

Status checks

PRs should trigger status checks as part of the CI/CD process - at a minimum a linting check and also a full test run if tests are written for the project. Usually this is set up at the beginning of the project but if not please rectify that.

Assignments

Make sure to assign the PR to relevant (or all) developers on your project, including the lead.

Note that using GitHub it’s possible to create a template for a PR, so that any PRs that are opened have content pre-filled. To do this, simply create a file in your repo at .github/pull_request_template.md with the following contents:

<!-- ## PULL REQUEST GUIDANCE ##
Please make sure this Pull Request covers a single unit of functionality, and has:
* A clear and concise title
* A link to the relevant issue
* A good description of the changes
* Any relevant updates to documentation or tests

For more, please read up on https://signal-noise.github.io/rfcs/text/0005-pull-requests#pull-request-contents
-->
Fixes # .

Description of changes, including rationale and any actions other developers should take to stay up to date.

Pull Request size

Each Pull Request should cover a single feature or unit of logic. In general this means you may need to make several PRs in a single day, and you ought not to go more than two days without a PR. If your features are bigger than this, you should probablty be breaking them down.

Each PR should contain a small amount of code relating to a single task or piece of functionality; this in turn makes it easier to review the PR properly and make it more likely to be merged in quickly and keep the project flowing.

In addition, this will make the eventual git history (and blame) much easier to navigate, which benefits anyone coming on to the project - including the you from the future, a year from now ;)

Reviewing a Pull Request

When reviewing code, look out for style, clarity and overall approach.

Specifically, review against the following criteria:

Comprehensibility

It’s important to ensure that the code is understandable, which may be as simple as adding some extra commenting, or may require refactoring.

Code style

Wherever possible, code should adhere to the relevant RFCs regarding style and code approaches, and the PR is a good place to catch errors. When RFCs are not adhered to, the exceptions should be justified and discussed in the PR description and comments.

Duplication

It’s often the case that coders miss work that others have done, resulting in duplication or different approaches to the same problem in the same project, which is always worth avoiding.

Performance

The PR is a great place to catch any potential performance or architectural issues before they’re deeply ingrained in the project structure.

Automations

No PR should go through without its status checks passing. If there is testing on the project, the review process should include checking that there are enough tests for the new functionality.

Pull Request reviewer responses

When posting your review think carefully about the type of response you post. The coder who opened the PR has taken time to achieve something and hopefully spent the time on making both their code and the PR clear and understandable.

Please be thoughtful about your comments, and address the work with specific comments, suggestions and reasoning. A Pull Request review is a chance for a developer to learn something, and for a team to come to new joint understandings and approaches.

In particular, do not close the PR without discussion with the person who opened it as this can result in repeated work and is very discouraging. We encourage discussion (either in the PR or in person / on a call) and learning, and a bad PR is a great opportunity to improve.

Using Request Changes

As a reviewer, Request Changes puts you in the middle of the process and is comparatively discouraging to other developers, since you are removing their ability to judge the seriousness of the impact of your comments (although of course this can be entirely appropriate).

If you’re not ready to approve, please only use Request Changes if: