Reviewing Patches

General guidelines

  • Write down your thoughts as you are reviewing, not afterwards.
    Keep a text editor open for typing in and write down your thoughts immediately. When you are done, clean it up and structure it.
  • Take a look at both the big picture and the details.
    Simply saying "I [don't] like this feature" or "-1" is of no use and is strongly discouraged. Similarly, diving into a patch and saying nothing but "there is a typo in function so and so" could be a waste of time as you could be continuing a patch that has no hope of being committed.
  • No patch can save the world, not even a ProjectPier core patch. If it works and does something useful on its own, then it is good to go. One of the worst things you can do is elaborate on other, equivalent approaches and suggest complex extensions to the patch. Additional features can be added later. A scalpel is often better than the Swiss Knife.
  • Don't stop reviewing at the first sign of trouble.

    If there are bugs, imagine how things should've worked. If there are usability problems, try to think of a better interface.

  • Pay attention to what the submitter has said about the patch.
    If some things are not clear, write down your questions. You should have a good idea of what the patch does and who it is for before you start.
  • Apply a patch to a SVN tree if you can.
    This allows you to immediately create an updated patch. Often it takes more time to describe small changes (like typos and code style) than it is to make those changes yourself.

Reviewing process

  • Challenge the proposed changes.
    What are the benefits? Are there disadvantages? Does it fit in the general ideas of ProjectPier? Does it make good use of our systems or does it re-invent the wheel? Does it belong in core? Can you see the changes being used elsewhere too?
  • Test the functionality.
    Does it do what it should? Does it work under different settings (e.g. clean URLs) and with different modules?
  • Examine the usability.
    If new pages are added, are they are in a logical place? If controls are added, are they intuitive and logical to use? Does it come with appropriate contextual help? Is the interface consistent?
  • Review the code.
    Is the code understandable and documented? Does it make appropriate use of APIs or does it re-invent the wheel? Does it conform to coding conventions? Is the functionality separated into logical pieces, or just mashed together? Are appropriate sections made themable?
  • Look for security issues.

    Is all user input appropriately filtered? Do APIs avoid or properly document security risks? Do all database queries properly prevent SQL injection?

  • Look for scalability issues.
    Do database queries hit indexes? Are efficient algorithms used? Are unit tests needed?

Authoring a patch review

When posting your issue follow-up, it is important to think critically and speak positively

  • Your goal is to propose an even better solution than the patch currently envisions. Do not post negative feedback without a corresponding solution. A good review helps a patch proceed, instead of obstructing it. It is a rare case that a submitted patch has absolutely no merit and warrants a purely negative review.
  • Post your findings in a clear and structured manner to the original issue. If you can, include an improved patch.