Why should you use pull requests?
We use pull requests at GDS because:
- it’s good practice to use them to review code
- it’s part of our accreditation - the extra pair of eyes provides a lightweight change management process, and shares responsibility for the code going live
- it’s good to spread knowledge and make others aware of changes
- it’s good to notify people (through email and GitHub notifications) who might
otherwise not be involved (we can even ‘mention’ specific people with
You should not:
- use PRs for architectural review - this should be done before you raise the pull request
- rely on someone else to spot and merge your PR - it’s your job to find a reviewer and get the code merged
A PR does not have to be all of the work - it’s okay to have follow-up PRs for a feature if a set of changes are complete enough to go live.
Guidance for each step
If you’re not sure how to do any of this, feel free to ask for help.
Opening a pull request
Before opening the pull request, make sure:
- you’re up to date with
mainso that your changes are easier to merge
- the title and description help the reviewer
- the title is succinct and descriptive, with detail in the description
- the description summarises your changes and includes useful links, for example, to a Pivotal ticket, ZenDesk ticket or related PR
- to describe any screenshots you use (for example, to show frontend changes) in the text, for accessibility reasons
- the title and description are accurate and relevant, as GitHub emails these (but not any subsequent changes) to people following the repo
- you explain or highlight any potentially contentious changes, and any testing that you have already done
The canonical description of changes should always be in the individual commits - pull requests are an artefact of GitHub, and we would lose that data if we switched away. You can refer to the commit message style guide.
Reviewing a request
Guidelines for review
It’s important to take time to review a pull request properly - the review stage is as important as writing the code in the first place. If you’re not sure how the individual wants their request reviewed, ask them before starting - they may prefer you to deliver some of the feedback in person or while pairing (especially if they’re less experienced).
If the code is good, or solves something in a clever way, say so. Call out individual bits of quality - it signposts good practice for others, and rewards the person submitting the request. Explicitly state what, if anything, is a blocker.
Communicate with others who may consider reviewing the PR
If you’re going to discuss some issues offline, you should indicate this in the PR so that no-one merges it in the meantime - “A few issues here - going to discuss offline” would be enough. When conversation has taken place elsewhere, summarise the conversation as a comment on the PR for the benefit of others.
If you look at a PR but do not feel comfortable merging it, say what you’ve looked at or not so other reviewers know the request has not been properly reviewed. If you’re committed to reviewing the request through to merging or closing, you can assign the PR to yourself.
Helpful things to consider while reviewing
When reviewing a PR, you should:
- try running the code - even if the tests pass, it might have bugs
- consider security when reviewing code, particularly where there is user input
- remember that a PR does not have to entirely solve the problem - if it adds value on its own, it is better to merge now rather than wait for the rest of the changes required
- make sure that any relevant documentation (
READMEfiles, things in the
docfolder) is up-to-date with the changes
You should address any comments flagged as blocking. This includes spelling or grammatical errors in documentation. If you’re changing something minor in an existing commit (for example, renaming a variable for clarity or adding a missing test), amend the existing commit (ask for help if you do not know how to do this). You should address major changes in a separate commit. Make sure you follow existing commit guidelines when addressing changes - “Addressed feedback” is not an acceptable commit message.
Explicitly comment that you have addressed all relevant comments to notify any watchers - you do not need to do this on a per-comment basis.
You should raise a separate, follow-up pull request for refactoring - it should never be considered a blocker.
Reviewing external pull requests
We sometimes receive pull requests from members of the public. While we should be polite to our colleagues, it’s even more important that we follow a few guidelines when dealing with people we do not know, some of whom will be doing this work in their own time.
When interacting with members of the public who have raised pull requests, you should:
- be positive – thank them for contributing, make this the first thing you say and thank them even if you are going to immediately reject their contribution
- make positive comments as well as suggestions for improvement – a list of just things to fix could be dispiriting
- make requests for improvement rather than telling them what to do (“We think it might be better the other way round, what do you think?” rather than “Swap the order of the logic”)
- be explicit. Remember that people do not always understand your intentions online
Handling the PR
You should communicate clearly about whether we’re interested in the feature or not. If it does not fit with our rationale for the codebase or we do not want to merge it for another reason, thank them and close the PR.
If it fits but we cannot merge it due to quality or style issues, then clearly state that we are interested in the feature, but there are barriers to merging the contribution in its current form.
It’s worth saying what improvements we’d like to see, but not putting the onus on the contributor to make them all. For example, we might add tests ourselves or work with them to add tests.
It’s okay to close PRs due to lack of activity, but in this case, invite people to reopen them if they pick things up again.
Do not comment on PRs, even to acknowledge them, at the weekend - we do not want to set an expectation that we support projects outside working hours.
Try to comment on the changed files rather than by commit, as the notifications are easier to follow.
If the contributor has not written a line in the changelog, write a line to describe it once their PR is merged.
Regardless of who has written the changelog entry, add a “Thank you @githubusername”. We do not usually add changelog notes for documentation, but if that comes from an external source, add a documentation credit at the end to thank them.