How to Review Pull Requests
| Supplementary Materials |
|---|
| The Do's and Don't's when Reviewing Pull Requests |
| Checklist |
| Sample Feedback |
| Flowchart* |
| Documentation |
Hi! Before we begin, allow me to thank you for taking your time to read this document! Pull requests are the heart and soul of the website project. By reviewing them, you ensure that the hackforla website is always up-to-date and provides a bug-free experience for our visitors.
As a member of the website team, you are expected to review pull requests. If the phrase, "pull request", inspires apprehension, worry not! This guide will equip you with the confidence you need to succeed at reviewing your first, and all subsequent, pull requests.
When multiple collaborators –ahem– collaborate on a project, changes are constantly made. Because not all changes are desirable, the review process allows the team to intercept and assess changes before they are merged with the website.
The review process starts when a collaborator creates a pull request, a fancy word meaning, "I, the collaborator, have made changes, so please review them!". As a reviewer, your goal is to examine the website's appearance and source code prior and following these changes. Specifically, you need to ensure the changes are: 1) applicable, 2) does not break the main website, and 3) clean (collectively know as the ABCs).
If the changes passes the ABC's, the pull request is approved for eventual merging with the website. Otherwise, you, as the reviewer, must leave feedback for the collaborator.
How to leave a comment or request changes
When changes fail to follow any of the ABC's, there are a couple of ways to leave feedback for the collaborator: leave a comment, request changes, or directly mark erroneous code. As required, all three can be done at once, but one will often suffice.
This is usually reserved for clarification purposes. To leave a comment, go to the Files changed tab, if not already there. Then click the green button labeled, "Review Changes". In the pop-up, leave a comment, and be sure to select the "Comment" radio button before you submit. You can also use the @ shortcut to notify others to your comment.
When changes fail at least one of the ABCs criteria, the best way to leave feedback is to request changes. As the name implies, when you request changes, you detail additional steps the collaborator must take before their changes passes the ABC's. To request changes from a collaborator, select the "Request Changes" radio button rather than "Comment" radio button. This flags the pull request, preventing merges until a reviewer approve the additional changes. When requesting changes, be sure that they are specific and actionable!
How to leave feedback directly on code changes
When leaving a comment or requesting changes, you also have the option of commenting directly on the source code, through Github's line comments feature. If you have the time, I would encourage you to use line comments as often as possible as they improve communication between reviewer and collaborator and minimize development time.
WIP
Parts of a pull request's branch
Before anything else, check that the pull request contains the correct branch. In other words, it must pass the following two checks:
- The commit into must be "hackforla:gh-pages"
- The commit from must be from the collaborator who opened the pulled request
If neither of the two criteria are met, then the collaborator might have made a mistake when making their pull request. Leave a comment stating that they have made the request with the incorrect branch and to redo the pull request with the correct branch.
Example of a linked issue
Every pull request comes with a post by the collaborator. Usually, a post contains: 1) a linked issue, 2) comments on the changes, and 3) screenshots of the changes. For this step, we will focus on the linked issue.
When a collaborator make changes, they implement them based on issues with the website. These issues are outlined in a separate post called a linked issue. Therefore, the linked issue provides important context that frames the collaborator's changes. Without it, there would be no way to tell whether changes are applicable!
Linked issues are usually in a Word #number (or in regex: [A-Za-z]* #\d*) format. They always link to a different post. Here are some, but not all, examples of a properly-formatted linked issue: Fixes #940, Fixed #1151, Resolves #1326, Closes #1444.
If the linked issue is not in the correct format, the review can still proceed, but do leave a comment on how to properly add a linked issue. However, if there the linked issue is missing, the review stops. Leave a comment for the collaborator to add a linked issue.
As an aside, we also ask that collaborators include before and after screenshots of their changes as part of the pull request. These images are there to help visualize the changes. If the images are not there, please gently remind the collaborator to include images before continuing with Step 2 of the review.
Example of an issue
Before a reviewer can understand the collaborator's changes, they must first have an firm grasp on the linked issue. Visit the linked issue. Read the post and the comments below, visit all relevant links, and click on any dropdowns. Approach the issue with the goal of understanding the issue, so that, later, you may accurately assess the collaborator's changes.
A good way to test your understanding is to restate or summarize the issue in your own words (or to your rubber duck, if desired). This helps in finding gaps to your knowledge of the issue and prevents you from blindly reviewing changes you have a hard time understanding.
If after reading the linked issue, you find yourself confused, do not panic! This can arise from unclear wording, or unfamiliar jargon. At this point you have several options:
- Research some of the unclear terms on your own
- Consult the person who brought up the issue (through a comment or our slack)
- Or, call on someone with more expertise (such as the person who wrote the issue) to review the issue with you
The easiest way to view the collaborator's changes is to see them for yourself! Outlined below are steps to download the collaborator's branch into your own version of the website.
Steps to downloading the collaborator's branch
Once the branch is installed, run the website and view it in the browser. You will also want to open our website in a new tab. Locate where the collaborator's changes on both sites and consider whether these changes address the issue. Some important questions to ask are:
- Are the changes applicable to the issue?
- Are there changes beyond those applicable to the issue?
- Does the website appear less user-friendly?
- Do the links and components on the page still work as intended?
In addition to viewing changes on your desktop browser, you must also assess these changes in multiple viewports (such as mobile or tablet), through your browser's developer mode.
An example of developer mode in Microsoft Edge (90.0.818.51)
Links to developer mode documentation in popular browsers (bookmark this for future reference 😊)
After viewing these changes in your browser, you might discover that the changes are not applicable or breaks the website. In that case, you must request changes, and detail what exactly went wrong. If everything seems fine, you may proceed to Step 4.
At this step, you assess whether the changes are applicable and clean. Click the Files changed tab on the pull request page and examine the code. Green highlights (or lines with a plus sign) represent additions to the base website's code while red (or lines with a - sign) are deletions. Although clean is a subjective term, do make sure that the changes follow typical coding style guidelines. Good questions to ask are:
- Can the changes be condensed?
- Can the collaborator add comments to clarify any complex changes made?
- Are there any drastic changes that seems like they do not belong?
- Are there changes that are missing?
If something about the code is off, leave a line comment in addition to a request changes review.
After viewing the changes in your browser and in the source code, the changes might still appear inadequate, erroneous, or incomplete. For example, the changes might have created an entirely new and unforeseen issue. Or there might have been a mistake in the original issue's wording which the collaborator did not caught. In such cases, leave a comment to discuss with the creator of the issue about your concerns about these changes. In some cases, you might also want the creator of the issue to review the pull request as well. Open the image below to see how to add them as a reviewer.
Manually adding another reviewer
How to approve a pull request
If after going through all the steps, and you find all the changes passes the ABC's, then congratulations! We are ready to approve of these changes. To approve, visit the Files changed tab and click the green "Review changes" button on the top right. Select "Approve" and leave something nice for the collaborator. Something as simple as a, "Great job! I love what you have done, insert name!", will really make someone's day.
WARNING: DO NOT MERGE THE PULL REQUEST. This responsibility is for the seniors on the team. If you have accidentally merged the changes with the site, alert a senior immediately so the site can revert to a previous version immediately.
DO NOT PRESS THIS BUTTON!
- Do: Check all files in the changed files tab.
- Do: Make sure all added/changed files are relevant to the issue.
- Do: When CSS changes, check a couple of pages rather than just the target page.
- Do: If changes are not apparent, try clearing the metadata first.
- Do: Leave encouraging, straightforward feedback.
- Do: Reward yourself! Reviewing code is not the easiest thing, so grab yourself a snack and try your best!
- Don't: Avoid reviewing pull requests.
- Don't: Be afraid to ask questions.
- Don't: Miss a step in this guide They are there for a reason!
- Don't: Merge the pull request, whether by accident (or malice, for that matter).
- Step 0: Is the pull request done with the correct branch?
- Step 1: Is there a linked issue?
- Step 2: Understand the linked issue.
- Step 3: View the changes in the browser.
- Step 4: Take a look at files changed tab.
- Step 5: Check for anything else.
- Step 6: Approve the pull request.
WIP
