Skip to content

CodeReview

Jump to bottom
Iva Kavcic edited this page Nov 30, 2021 · 7 revisions

Requesting a Code Review

In order to request a code review you must create a pull request (PR) on GitHub.

Once you are happy that your PR is ready for review please give it the "ready for review" label and request a review from one or more people: Andy [andrewcoughtrie], Maff [mo-mglover] or Iva [TeranIvy]. Although you can request a review from multiple people, only one review actually needs to be performed.

A review is performed in GitHub by creating comments on points in the code that need attention. The PR author may respond to these comments and the result is a 'conversation'. It is up to the reviewer to decide when a conversation can be marked as 'resolved' (i.e. the point has been dealt with to their satisfaction).

Guidelines for Performing a Profiler Code Review

  1. Replace the "ready for review" label on the PR with the "under review" label. This helps to prevent multiple people attempting to review the same thing and also makes it clear at what stage in the workflow the PR is.
  2. Check that branch is up-to-date with master (the pull request should report that the branch can be "merged automatically"). If not, return to developer for them to bring it up-to-date (this can be done when requesting changes).
  3. Check that there are no TODOs in the code that refer to the current issue/PR.
  4. Use the "Files changed" tab on the pull request to review all code changes. Check that all code modifications are well commented, easy to understand and that the code is correct. Comments and requests for changes may be made in-line on the "Files changed" tab. This makes it easier for the developer to see which part of the code is being discussed.
  5. If there are suggested or/and requested code changes then return the PR developer to address the review remarks.
  6. Check that the copyright/author details are correct in any modified files.

Once the review is complete there are two options:

  1. If the reviewer is happy with the pull request then they can proceed to merge the branch onto master (see below).
  2. If the reviewer is requesting changes, then the "under review" label on the PR should be replaced by "reviewed with actions" and it is then up to the original developer to address the reviewer's concerns.

Merging a Branch to Master

Once a pull request has passed code review, the code reviewer should merge the associated branch onto master:

  1. Check out the most recent version of the branch.
  2. Check that all tests pass in this version (sanity check).
  3. Update the CHANGELOG.md in the top-level directory (using the text describing the Issue or PR).
  4. Commit these changes and push branch to GitHub.
  5. On the pull-request page on GitHub, request the branch be merged to master. Use Squash and merge option.
  6. Check out the latest master branch and check that all tests still pass!
  7. Delete the original branch.
  8. If there is an Issue associated with the pull request, close it (note, this can be done automatically by linking the pull request to the associated issue).
Clone this wiki locally