Guidance for reviewing a pull request

This section details how we collaborate by reviewing pull requests.

Why do we need code reviews?

Code reviews are a requirement and should happen before the code in a branch is merged into dev.

PRs and code reviews...

Ensure high code quality across the whole team;
Increase transparency;
Ensure logic and methodology implementation are correct;
Help with bug identification;
Are an industry standard - so great skills to learn;
Socialise how different parts of the code-base work.

Steps to take to make a review on Github

Follow these steps to make a review on GitHub. Take a look at the full how to page.

To access a pull request assigned to you from a particular project, first navigate to the repository then click Pull requests. In the list of pull requests, click the pull request you'd like to review. Alternatively, you can see all pull requests assigned to you by checking your GitHub notifications page under reviews requested. Once in the pull request, click Files changed.

Hover over the line of code where you'd like to add a comment, and click the blue comment icon. To add a comment on multiple lines, click and drag to select the range of lines, then click the blue comment icon.

In the comment field, type your comment.

Optionally, to suggest a specific change to the line or lines, click the plus/minus symbol at the top of the comments box, then edit the text within the suggestion block.

When you're done, click Start a review. If you have already started a review, you can click Add review comment. Before you submit your review, your line comments are pending and only visible to you. You can edit pending comments anytime before you submit your review.

To cancel a pending review, including all of its pending comments, click Review changes above the changed code, then click Cancel review.

See this nice example of a summary post.

When submitting your review you are given the following options (just above the Submit Review button)that gives the receiver an indication of the type of review and what there next steps should be.

Comment: The review contains suggestions for changes and therefore the code is not explicitly approved until either the changes are made or discussions have been resolved and a way forward agreed.
Approve: The review has been carried out, comments provided and suggestions for future issues made, but no additional changes are required so the PR can be merged immediately.
Request changes: The review contains changes which must be made before merging - e.g. fixes breaking changes introduced in another branch.

Reviewing tips

Clone and run the code or individual steps and functions
Question or comment on anything which isn’t
- a) obvious b) nice c) efficient d) standard
Make both general comments and comment on specific lines of code
Use the ‘Suggestion’ feature extensively (although also note it is not your responsibility to fully re-write sections of code, especially those which might need a substantial rethink)
Explain the rationale for a suggestion and provide links (e.g. to library documentation or StackOverflow)
Comment on things you think are nice as well or that you learned from seeing
Just like you might with writing code: read the docs > have a go > ask for help
Try not to ask for big restructuring unless absolutely necessary. Or consider splitting out bigger suggestions into separate issues and flag it in the PR so it doesn’t hold up merging a PR that is time sensitive. Ideally, most questions about approach and methodology should be resolved through discussion before a PR is reviewed!

Optional reading material - A code review your colleagues would thank you for - 15 topics to consider as you review code in data science

Why bad reviews happen and what we can do

Can’t find anything wrong in the code You should be able to find something to comment on or question. A lack of clarity in code or documentation is worthy of comment. Say what you have done (e.g. “Checked out the repo, ran this module using python 3.8, found that there was an issue, so reverted to python 3.7 and everything is running fine”)
Don’t feel qualified or don’t want to reveal own ignorance We are all learning! Asking questions or clarifications from the author is fine. Chat or call if needed. You can make suggestions without the certainty of being right. This might prompt the author to rethink their approach or prompt them for a useful justification. Use '#dev' extensively. If you can’t figure out something or an improvement, use the team. The chances are that a useful conversation will be triggered and you, the author and others will learn. Really stuck? Ask a colleague for direct help reviewing a particular section.
Trying to be nice You can be critical and nice at the same time! This is about bringing everyone up, not individuals down. Critical can mean asking someone to refactor from the ground up. This might take time but is a valuable learning experience.

Example review comments

Suggested changed

Giving an example of the change you are suggesting can help make it really clear what the change looks like in their code.

Code is fine, but needs a bit of explanation.

Review can cover rationale for an approach or methodological suggestions.

Nice comments about the persons code

Code review doesn’t have to be a mistakes list. If you stumble upon a good place, nice decision, useful piece — communicate your appreciation.

Summary post

A nice example of a review summary that breaks down what the reviewer has done in the review. It also includes some suggestions at the bottom but highlights they are not mandatory for this PR to be merged (but could be split out into a new issue to action in the future).

Very short non-descriptive summaries such as "seems fine" should be avoided unless the change is trivial or you're on the last iteration of multiple reviews.