Pull requests have two parties involved: the person who writes and opens the pull request, and the person (or people) who reviews it. In this section, you put yourself in the position of a reviewer of a pull request.
Reviewing a pull request is a very active activity. It takes a lot of focus and attention to do it well. It doesn’t serve the folks submitting pull requests if you just take a cursory look at it, comment LGTM (Looks Good To Me), and don’t provide quality feedback.
Sometimes, a cursory look is all you have time for. In that case, make it clear what you did and didn’t review and suggest that someone else provide a more detailed review.
When someone adds you as a reviewer, you’ll receive a notification about the request, typically via email or on your notification bell at the top right of GitHub. com. If you visit the pull request, you’ll see a banner message, as shown in Figure 8-8.
WARNING: Don’t click the Add your review button just yet. That takes you to a dialog box to write a write-up of your review. Only do that when you’re ready to complete your review.
1. Reviewing the Conversation tab
When you review a pull request, the first thing to do is read through the contents of the Conversation tab for the pull request. Make sure that you understand the purpose of the pull request and why it’s necessary.
At the bottom of the conversation is a section that displays the checks that GitHub runs against the repository if there are any. If no checks are set up, you see the message Continuous integration has not been set up. The status of these checks is the next thing you should check. Many repositories will have several checks, such as does the code compile, did all the tests pass, and so on.
If any of these tests fail, you shouldn’t spend any more time reviewing the code. The person submitting the pull request can see that their pull request doesn’t pass all the checks, and it’s up to them to fix it.
TIP: Even though the person submitting a pull request should be able to see that the checks have failed, sometimes they don’t stick around long enough after creating the pull request to see that. You may want to add a gentle note that mentions the person submitting the pull request and informs them that the checks have failed and he should try fixing the problems and push his changes to the pull request branch again.
Figure 8-9 shows an example from the GitHub Desktop open source project of a pull request that fails the continuous integration (CI) builds.
2. Reviewing the changed files
Assuming you’re happy with the contents of the Conversation tab and all the checks pass, it’s time to get into nitty gritty and review all the file changes. Click the Files changed tab to see all the changes in the pull request.
At this point, you should review the code for things like
» Clarity: Is the code easy to read and understand? Could it be made more clear? there appropriate comments throughout the code? Obtuse, hard-to- understand code becomes a maintenance nightmare down the road.
» Correctness: Does the code do what the pull request says it does? Are there any glaring bugs? Are there any errors of omission? Are there any tests missing?
» Security: Related to the previous item, a security review requires a specific mindset. Ideally, you work with security experts who can help review the code for security. The idea here is to think about all the ways a bad actor could attempt to attack the code. There are many frameworks for doing security review, such as STRIDE. You should also think about how bad actors can use the code to harm other users. Does the code protect users privacy? Does it ask for consent to take actions on behalf of users.
» Conventions and idioms: Just because code is correct, it doesn’t mean it’s necessarily idiomatic. A code review is a good place to teach and learn conventions and idioms specific to a project.
In the last section, we mention conventions. By conventions, we mean common approaches to accomplishing a task. For example, if your project has a certain approach to querying the database, make sure the code follows that approach.
WARNING: One thing to note is we don’t cover code style issues in the list of things to review. A code review should not cover nitpicks like whether a curly brace goes on its own line or not. An overly pedantic nitpicky code review does not set a friendly and collaborative tone. Depending on the context, you can just fix these things yourself or better yet, use automated tools, such as a linter or prettifier, to do it. You’ll save everyone a lot of time and headaches.
3. Commenting on code
When reviewing changed files, you can add comments to specific lines of code to indicate a problem with the code, add a suggestion to make it better, or celebrate someone’s awesome code writing skills with a :sparkle:.
TIP: Positive and encouraging comments set for a welcoming and collaborative tones. Maintainers often forget how daunting it can be to contribute code to a project for the first time. Don’t be stingy with the :sparkle: emojis!
To comment on code:
- Hover your mouse over the line of code.
A blue square with a plus sign appears next to the line number.
- Click the square to reveal a comment form for that specific line of code, as shown in Figure 8-10.
The comment form supports the same things issues and pull requests do, such as mentions, Markdown, and, of course, emoji.
At this point, you can choose to click Add single comment or Start a review.
Clicking Add single comment immediately adds your comment to the pull request and sends out any notifications. This can be useful when making a quick one-off comment. In most cases, we recommend against this approach as it doesn’t lead to well considered and thoughtful code reviews.
- Click Start a review instead to create a review and add the comment as a pending comment to the review, as shown in Figure 8-11.
The pending label indicates that you’re the only one who can see the comment so far. By starting a review in this manner, you can continue to add (and edit) pending comments as you review the code and only publish them when you’re completely satisfied with the review.
TIP: The reason we recommend starting a review as opposed to adding single comments as you go is a review leads to a more coherent and helpful code review. A common occurrence when reviewing code is that something you read later in the review makes you realize a comment you made earlier should be updated or even deleted. A review lets you make those adjustments before you send anything to the author. This option gives you an opportunity to review your review before publishing it.
4. Suggesting changes
Sometimes as you review code, you come across a section of code where you feel like it’d be faster to just fix the code than try to explain in words what should be fixed.
Or perhaps you run into a lot of small errors, such as typos, where commenting on each one produces more noise than just fixing them. For these situations, GitHub supports suggesting a change via a pull request comment that the author can apply with a click.
To suggest a change:
- Opening the comment form for the line of code you want to change.
When you bring up the comment form, you see an icon with a + and – symbol. When you hover over the symbol, the tooltip describes the purpose of this button (see Figure 8-12). The tooltip also describes a keyboard shortcut you can use to suggest a change, CMD + G.
- Click the Suggest changes button.
GitHub adds a suggestion block into the body of your comment with the current contents of the line you want to change.
3. Change the contents of the block to suggest what the final result should be.
Figure 8-13 shows an example where I suggest a change to add the word “Instructions” to the line
TIP: Anything you write outside of the suggestion block in the comment is not included in the suggested change. This lets you provide a reason for the suggested change. Except for truly simple or self-explanatory suggestions, we recommend always providing this extra context.
After you create a comment with a suggested change, GitHub displays the change as a small diff within the comment. If the person viewing the suggestion has commit access, they will see an option to commit the suggested change (see Figure 8-14). They will also thank you for saving them time!
5. Finishing the review
After you have made all your suggestions, you can finish the review so the author receives all your valuable feedback and can start to address it. To finalize the review, click the Finish your review button at the bottom of any pending comment (refer to Figure 8-11).
You see a form where you can write your overall summary about the pull request. This form is an opportunity to bring up any review comments that are not specific to any lines of code. It is also a good opportunity to offer some general praise, raise broad concerns, suggest follow-up actions, and so on.
After you type your comment, check one of the options to indicate your position on the pull request. Figure 8-15 shows the comment form with the review options.
TIP: If you’re reviewing your own pull request, the only option available is to Comment on the pull request.
Your choice may determine whether this pull request can be merged into the default branch. Whether your choice blocks a merge depends on how the branch protection rules in place for the repository. For example, some repositories may require a certain number of approved reviews before the pull request can be merged. Go to Settings o Branches to manage branch protection rules.
Source: Guthals Sarah, Haack Phil (2019), GitHub for Dummies, Wiley.