Code reviews
All code that is intended to be merged to the master, release, feature, or bucket branches must first undergo code review.
Code reviews must be done by a core product engineer.
Opening a Pull Request
So…you have some code that you want to merge? A Pull Request is what you need to make that happen! There are a few important things to keep in mind:
- An approachable title
- A good description
- Including an artifact
- Reference to a ticket
- A changelog entry
- Adding a label
An approachable title
Use a title that is descriptive and brief. It should be clear what the goal of the pull request is and should be human readable. Easy peasy, lemon squeezy!
A good description
Including a solid description is key to conveying what your code is aiming to accomplish. Remember, you are the one with the most knowledge about the code you are working on and the reviewer may be coming in somewhat blind, so be sure to be as thorough as possible. Some things you may consider including:
- The theme of what you are attempting to accomplish
- Potential problem areas
- Tips for reviewing efficiently
- Any other considerations that should be made while reviewing
Including an artifact
Every pull request should include an artifact of some sort. This can be a screenshot, a screencast, the passing tests, a snippet of data, or a description of why an artifact isn’t needed.
Note: If, through the code review process, you are asked to make significant changes, redoing your artifact is a good idea.
Reference to a ticket
Almost all tickets should be linked to Jira via a ticket reference. You can do so by adding the following to your pull request description: :ticket: [TICKET-123]. If you are an outside contributor, don’t worry about this piece. We’ll create a ticket and add a reference on your behalf!
A changelog entry
In many cases, a changelog entry is needed. Please follow the guidelines on our Changelogs page for best practices.
Add some labels
When submitting a code review, be sure to add the code review label to your Pull Request as it will ensure that the PR is placed in the proper Pull Request report section in Slack.
Performing a review
Reviewers should consider the following:
- Does this code meet our standards?
- Is the pull request against the correct branch?
- Does the solution make sense?
- Will this solution lead to other problems?
- Backwards Compaitiblity
- Performance
- Security
- Language Typos
Before starting your review
Super important: You should not begin your review until the following things are true:
- There is a description on the PR, hopefully with a ticket reference
- There is an artifact on the PR and you have reviewed it
- Tests are passing
Calling out a problem
Sometimes code submitted for code review will introduce bugs, performance issues, security problems, etc. When that happens, be sure to call out the issue in a comment, giving suggestions where appropriate. After commenting, be sure to label the PR with the bug label so that the submitter (and PR reports) know the type of fix that is required.
Making a suggestion
While you are reviewing code, you may come across something that could be better or might need an alternate approach. Be sure to leave a thorough comment explaining your thoughts including whether a change is optional or not. After leaving your comment(s), add the enhance label to the PR.
Asking for clarification
Pull requests can sometimes have code that sparks a question, has a technique that you’d love clarification on, or brings up a change that may have impact that needs discussion. Be sure to add a comment with a thorough explanation and/or question, citing code or documentation as needed. Then, be sure to throw the question label on the PR.
Flagging other issues
There are other important things to look for in a PR and there are labels that flag those misses. Be sure to use them as appropriate:
needs artifact– The PR does not have an acceptable artifact.needs changelog– The PR requires a changelog entry but is missing one.needs release– The PR is not pull requested against an appropriate release, feature, or bucket branch.needs tests– The body of work requires additions to our automated tests.needs ticket– The PR is missing a ticket reference to Jira.translation– The PR has a translation issue that needs addressing.
Approving or deferring
If a code looks good to you, there are two potential paths:
- You have the context needed to approve the merge – if so, comment as such, mark the PR as Approved, and add the
mergelabel onto the PR. - You do not feel like you have the context or expertise to approve the merge – if this is the case, comment on the PR indicating that the code looks good from what you can tell and suggest that another engineer (feel free to @ message them) should do the final review.
Merging a Pull Request
Once a PR gets the approval to merge, double-check that everything is in order: it is Pull Requested against the correct branch, it includes a changelog entry where needed, and you believe the reviewer had the context necessary to approve. If all of the above are satisfactory, then the PR is ready to merge!
Who should merge the PR? Typically the person that opened the pull request. But it doesn’t necessarily have to be if you have the context to update the associated ticket in Jira.
Merge and then delete the branch. Promptly follow up by updating the associated ticket in Jira. A good practice is to take the artifact from the PR and add it to the Jira ticket as a comment and/or under the Testing Instructions section.