webchick.net

very helpful lioness

Diaries of a Core Maintainer #3: Patch reviewers are *not*, in fact, clairvoyant!

Wed, 09/24/2008 - 13:14 -- webchick

Here's another tip for those who are looking for more reviews of their patches and faster commits: describe what the heck your patch does! :)

A simple enough premise, and It sounds obvious, right? But you'd be surprised how few patches are accompanied by a good, solid list of changes.

For a really bad example of this, allow me to pick on our #1 core patch contributor for a moment. Witness http://drupal.org/node/259412. The issue title is "Total module system revamp." The description? Two words: "Drupal diet." Accompanied by a 30K patch that removes and adds all manner of things. Please DO NOT do this. :P Doing this means that anyone who wants to review that patch (or commit it) has to sit down for at least an uninterrupted hour and sift through the diff to figure out what's even going on, before they can even begin to do the actual review to answer things like, "Is this the best way to solve this? Are there additional places we could make improvements? Does the code introduce logic errors?" and so on. Since this is a daunting effort, it means that often times reviewers simply don't bother looking any further at such patches, and so they end up rotting in the queue for months on end.

Most patches aren't this extreme, but they could almost all universally stand to be improved. Let's try to make sure every patch in the queue has the following:

  • A descriptive, well-scoped title. Rather than "hook_help() is busted and lame," go for "Help from hook_help displayed on 403 pages." If you can't adequately sum up what the patch does in 50 characters or less, it should probably be broken up into two or more separate patches.
  • (for features) The "philosophy" behind the change. We need a hook_how_now_brown_cow(), and STAT! Really? Why? Demonstrate what's wrong with the way Drupal currently behaves. Come up with clear use cases, and how this feature would make life better for people who have it. Talk about why your proposed solution is better than other solutions that might be proposed to solve the same use case.
  • (for bugs) A clear description of the bug, with steps to reproduce. It's amazing how fast it is to commit a patch that fixes a bug when it contains specific instructions on how to see it, which can verify that the patch fixes it. It's also amazing how NOT fast it is to commit a patch that is missing this critical information.
  • A summary of changes made by the patch. The bigger your patch is, the more critical it is that it contains this in order for reviewers to be able to get their jobs done. It's amazing how much difference a well-written patch description can make in terms of helping reviewers quickly grasp an issue.
  • A summary of discussion to date. Particularly in those gnarly 100-200 reply issues, but even in those that have more than 10 replies, summarizing what has been talked about and changed / not changed in the patch as a result really helps those who have not been involved in the patch from the beginning get up to speed quickly without spending lots of time reading comments that were rendered obsolete by the newest version of the patch. Try to do this (as well as the summary of changes, above) each major patch revision, or every 25-30 non-"subscribe" replies, whichever comes first. It'll help keep down the people bringing up things that were already dealt with, and also shave hours of time off of a patch reviewer and committer.

Thanks for doing your part to make patch reviews quick and easy. :)

Comments

Submitted by Steven Wittens (not verified) on

This advice goes the other way too. As a patch reviewer, make sure you look at everything that is provided: context, explanation, screenshots and the patch itself. First thing I usually did was scroll through the .patch file to get a feel for what it is doing and how. This is even more important if you're not a committer, because your review is a recommendation to committers and should be as informed and balanced as possible.

Not doing so might lead to various microwave related injuries.

Submitted by webchick on

Looks like a follow-up post is in order about how to be a good patch reviewer. Thanks, Steven. :)