Examine the diff and comment on it, a line at a time

That’s not very constructive, Lorna. Do you have a bit more advice for us? Since you asked so nicely, yes I do!

How to Review a Pull Request

First, read the description. If there isn’t one, close the pull request. You might need to go looking for an original task or story but it’s vital that you read the words there and reflect on what problem this change should solve or what feature it should introduce. The code might be faultless but still miss the point or not catch an important edge case: as a reviewer, that’s your job.

Now take the code and run it locally. If you haven’t run the code, why are you accepting it into production? Even if you aren’t expecting reviewers to also merge the code, an approving review is taking responsibility for that change going live. If it’s prose, you still need to see how it renders, etc. Casting your eyes over a diff is never, ever enough, if you’re serious about what you do, you’ll see the change in context and “poke” at it. I’m sure you do have an excellent build process with tests and whatever – are they better than a curious human checking something for cracks?

When you can answer “yes” to the following questions, you can proceed:

• Yes, I am happy that this is a useful feature/change, that I understand its purpose and it is fit for that purpose.
• Yes this change works as expected, even in the not-success case, even with unexpected data, even with missing configuration.

OK now you can read the code diff. My final piece of advice: try not to add low-level idle commentary on other people’s code when it really doesn’t matter which way things are done. For example: nitpicking use of one approach over another but then allowing the code to merge anyway is IMO very unhelpful. However asking for a confusing/unclear/abbreviated variable to be renamed is probably valuable for the longer term.

Beware the “Helpful” Tools

GitHub is the worst offender for encouraging really awful pull request review processes. When you go to “leave your review”, you are immediately transported to the diff. This is exactly where you should not start! Looking at the diff encourages commentary on whitespace. It does not encourage consideration of what is missing from this change request and it also does not encourage trying the code, asking questions, etc. I understand they mean well and for static websites on github pages, perhaps this is enough and the “big green button” is a helpful addition to the workflow. Most of the projects I work are a bit more complicated than that and I encourage developers everywhere to use their brains as well as the helpful tools to provide the best-quality feedback on their peers’ changes that they possibly can.

(This blog post also available in talk form but I’m not sure where to submit it to. Suggestions welcome in the comments) EDIT: it turned into “A Meritocracy of Pull Requests” at PHP Yorkshire!