Reviewing a pull request

Someone else than the author should review the pull request. Naturally, the author is confident the thing is perfect and that there is no way it could be improved upon! But let's entertain the thought that someone with a fresh pair of eyes could come up with improvement ideas. In this case, that someone is Bob!

Quis custodiet ipsos custodes? Who will guard the guards themselves? - Roman poet Juvenal

Look at it 🔎 🦆

The pull request review process depends on the pull request, the requirements imposed by the project, the team's standards, conventions, etc.

One way to start the review is looking at the pull request. Read the title and the description. After reading those, do you have an idea of what the thing is about? How will it affect the robot if you merge the changes? If you feel unsure of the consequences of merging the changes, ask the author. The value of your review will be higher if you understand the thing you are looking at.

When I see a bird that walks like a duck and swims like a duck and quacks like a duck, I call that bird a duck. - Indiana poet James Whitcomb Riley

Fair enough. But is the duck healthy? Feel the pulse. Is the duck clean? Wash, if not. Does it bark? No? Good.

Bob sees the new pull request and decides to review it. The title and description are clear. Bob thinks he understands the reasoning right away! He views the changes using the Files changed tab in GitHub.

Do you see anything that could be improved? Hint: The vault file path. 😉 It would be better to commit a generic path such as /Users/<username>/vault.json. But even Bob is not perfect, so we let it slip this time!

Think about it

Do the changes make sense? Do they feel appropriate and necessary? Will they make the robot better? Or at least less bad? Do you understand the reasoning behind the changes?

Bob agrees that moving the credentials away from the robot is a good idea! This will improve security. It will add some complexity compared to the hard-coded credentials, but in this case, the added complexity is justified.

Try it out (both locally and in Control Room)

Take the duck for a walk. Check for a limp. Throw it in a pond and see if it floats. Listen to the quack. Testing the changes on your computer might reveal issues such as:

• Forgot to include all the required files in the pull request -> Works only on the author's computer.
• Works only on macOS, but not on Windows (if the robot is meant to be platform-independent).
• Does not work at all (author's cat walked on the keyboard prior to the commit). 🐈
• Setup instructions are lacking or missing important bits.

Bob wants to give the robot a try on his computer to make sure it works. He navigates to the pull request in GitHub, scrolls down the page, and clicks on the command line instructions link:

Bob has already cloned the repository on his computer. He navigates to the root of the repository. Bob is currently on the master branch. He runs git pull to make sure his local master branch is up-to-date with the remote master branch. Bob then follows the command-line instructions for getting the remove-credentials branch on his computer:

• git fetch origin fetches the metadata from the remote repository. This metadata includes information about the branches on the remote. After fetching the metadata, it is possible to checkout those branches!

• git checkout -b remove-credentials origin/remove-credentials checks out the remove-credentials from the remote (origin) to Bob's computer and uses the same name (remove-credentials) for the local branch.

• git merge master merges any changes from the local master branch on Bob's computer to the remove-credentials branch (also on Bob's computer). This is a good practice to follow to make sure all merged code in the master branch is also included when testing the remove-credentials branch since the changes in the master branch might affect how the remove-credentials branch functions.

Now Bob has the remove-credentials branch on his computer and can test-drive the robot! Bob could have used a graphical Git client for these operations, too, but he wants to learn the command-line way! Bob follows the instructions in the pull request and manages to run the robot. Yay! He pushes the robot to the development workspace in Control Room to make sure it also works there. Since the vault has already been set up, the robot works without a hitch! Something feels a bit off, though. 🤔

Improve it

Minor typos and smaller issues are often best addressed by the reviewer to reduce the feedback loop time of pointing out an issue and waiting for the author to improve the implementation. If it is a small thing, just fix it! 😅

Bob feels it might be useful to include the robot setup instructions in the repository README.md file, since the pull request will be eventually merged and closed, and the instructions will be difficult to find if a new developer joins the project. Bob's boss asks him to brew some coffee, so Bob does not have time to improve the instructions himself.

Request changes

Larger architectural or conceptual issues might require deeper discussions before proceeding with any changes. These are often better implemented by the author herself. You can write your review in GitHub and mark the status as Request changes. This will flag the pull request so that the author knows that some modifications are needed.

Since Bob is completing important chores, he marks his review as Request changes.

Approve it

If there were no issues or only minor things that the reviewer decided to fix while reviewing (to avoid the feedback delay), the review status could be set as Approve.

Submit review

The final step of the review process is to submit the review, fetch a cup of ☕️, and wait for the author to do all the heavy lifting!

What we learned

• Having someone else to review your changes is a great idea.
• Try to understand the changes and the reasoning for doing them. If you don't understand, ask the author.
• Test the changes. Eyeballing is not enough for finding potential issues.
• Test both locally and in Control Room.
• Fix easy-to-deal-with things as you review to reduce the unnecessary feedback loop delay.
• Request changes for things that are not straightforward to fix.
• Discuss potential architecture issues with the author. You may come up with something that works better!
• GitHub provides excellent tools for reviewing and discussing the changes.