Skip to main content

Pull Requests

Once a site has gone live the master branch will be locked, and all code merged into master can only be done via a PR. This applies to all retainer and growth clients.

Creating PRs​

To create a PR go to the project in GitHub, click the Pull Requests tab in the top, then click the green New Pull Request button.

Select your feature or hotfix branch from the compare dropdown, leaving the base dropdown as master, then click Create Pull Request

If you have been working on a feature recently it may show at the top of the page in a yellow box, pre-selecting your branch to compare.

PRs should be reviewed by another developer from your pod, in order to check for any mistakes, as well as to suggest improvements. Once the PR has been created, paste a link to the PR in your pod Slack channel and ask someone to review it. If no-one takes it up, the pod lead should designate a developer.

If the PR is urgent and no-one is available to review it we are currently allowing developers to self-approve their PRs, though you must give a final quick read over your code before doing so.

In the future we will enforce mandatory peer reviews from a separate developer, so it is a good idea to get used to this process.

Once your PR has been approved click the green Merge or Squash and Merge button, then click the Delete Branch button to keep the repository tidy.

Reviewing PRs​

If a developer has asked you to review their PR you can find it in the Pull Requests section of the repo or via the link sent by the developer.

Click the Files Changed tab to see the final list of all of the changes in the branch. Read through the code and try to spot any obvious errors or causes for concern.

If you spot an issue, hover over the line and click the blue plus on the left hand side. This will open a textarea where you can leave a comment to the developer. Once you have written your comment, click Start a Review

When leaving a comment:

  • Make sure that any comments you leave are constructive, rather than critical. If there is something that can be done better, give an example and explain why this method is preferable.

  • Try to avoid opinionated comments - the point of a PR is to spot errors rather than telling a developer that you prefer to do it in a different way. Of course if there is an accepted standard that is not being adhered to then that is okay to request a change.

  • Be polite. Remember that this is someone's work, do not insult or belittle the other developer.

Once you have finished reviewing the code, click the green Review Changes button in the top right hand corner. If there are no comments, choose Approve, If changes are required select Request Changes then click Submit Review.

The developer will then be notified (though it's worth pinging them on Slack). If no changes are requested they can then merge the branch and deploy it. If changes have been requested they should review the comments, and either make the requested changes, or respond to the comments.

Just because a developer has requested a change, you don't necessarily have to do it. You likely have more context on the issue, so there may be a reason you've done it in this specific way.

Benefits​

PRs are beneficial to both the reviewer and the reviewee, as well as to the company.

The main benefit of PRs is in reducing code errors - by having another developer read your code they may identify a problem before the code is deployed.

When you review someone's code you may learn new techniques from the code, and alternatively, you may be able to suggest improvements to the other developer, it's a two way street.

PRs should be done by all developers, not just senior developers. If you have seen an interesting piece of code and would like to know more, feel free to leave a comment (rather than a review) asking for more information.