Pull requests

Pull requests (PRs) let you tell others about changes you’ve pushed to a branch in a repository on GitHub.

Why might you use pull requests?

We might use PRs at MHCLG to:

enable code review from colleagues
provide a lightweight change management process
share responsibility for the code going live
spread knowledge and make others aware of changes
notify people outside the immediate group of reviewers

Depending on the work flow of your project you might choose not to use pull requests. There are teams in the department that use trunk-based development instead of pull requests.

Cautionary notes

You should not use PRs for architectural review, as you should do the architectural review before raising the PR.

It is your responsibility to make sure you get reviews for your PRs and merge them. You should not rely on someone else noticing, reviewing, and merging your PR for you.

You do not need to merge all changes for a piece of work in a single PR. You should break down the changes into several PRs to get feedback earlier.

Guidance for each step

Feel free to ask for help on Slack (for example, #com-developers) if you are unsure how to do any of these things.

Opening a pull request

You should make sure that:

your local repository has the most recent changes on the target branch
the title and description of the PR are clear and accurately represent your changes
the title and description reference the source of the change, which could be a Jira card
the description contains links to any related PRs
any screenshots have text descriptions for accessibility
any contentious changes or side effects have clear descriptions of the pros and cons

PRs are an implementation of change review and depend on the continued support of a repository hosting provider such as GitHub. This means that we would lose data contained in PRs if we switched providers.

As such, the canonical description of changes should be in the commit messages. You can refer to the commit message style guide.

Reviewing a request

Taking time to properly review a PR is as important as making the change itself, and it is crucial that we show compassion when critiquing someone else’s work.

April Wensel has a talk about Compassionate-Yet-Candid Code Reviews.

It suggests 3 key questions to ask ourselves when reviewing or commenting on someone else’s work:

Is it true?
Is it necessary?
Is it kind?

Guidelines for review

Ask the PR raiser if you do not know how they want their change reviewed; they may prefer you to deliver feedback in person or while pairing.

Call out and celebrate good code, content, and design choices. This will also highlight good practice for others.

State explicitly if there are blocking issues.

Communicate with others who may consider reviewing the PR

Comment in the PR when discussions about the change are happening “offline” (outside the PR itself). Summarise any “offline” conversations as a comment in the PR for the benefit of others.

Comment in the PR if you have not finished reviewing the change. You might want to directly inform the PR raiser if they are expecting you to finish your review by a specific point.

Helpful things to consider while reviewing

Pull the branch to your local repository and try running the code; even if the tests pass, it might have bugs or unexpected side effects.

Consider the security and privacy implications of the change, paying special attention to where there is unsanitised user input or logging.

Make sure that documentation, including the README, is consistent with the code changes.

Addressing comments

You should address any comments flagged as blocking. This includes spelling and grammar mistakes in documentation.

You should amend minor changes into a previous commit, such as renaming a variable or adding a missing test. You should address major changes in a separate commit.

Make sure you follow existing commit guidelines when addressing changes. “Addressed feedback” is not a helpful commit message for future contributors.

Add a comment to show you have addressed all relevant comments.

A request for refactoring, while encouraged, should not block a merge.

Reviewing PRs from members of the public

Pull requests from members of the public should be addressed according to the guidance set out in The GDS Way.