Difference between revisions of "Dev Reviewing Guidelines"
(mention BML) |
(→Providing Feedback) |
||
| Line 21: | Line 21: | ||
On the other hand, if you've found a problem with the code, you should change the '?' to a '-' and provide the author details of whatever needs to be fixed. | On the other hand, if you've found a problem with the code, you should change the '?' to a '-' and provide the author details of whatever needs to be fixed. | ||
| − | Once a patch has been given the thumbs up by a reviewer, it should be committed by one of the developers with commit privileges, as outlined in the [[Dev Committing Guidelines]]. Make sure the commit flag on the patch is set to '?' so that it shows up in the queue of commit requests. | + | Once a patch has been given the thumbs up by a reviewer, it should be committed by one of the developers with commit privileges, as outlined in the [[Dev Committing Guidelines]]. Make sure the commit flag on the patch is set to '?' so that it shows up in the queue of commit requests, unless indicated otherwise. |
Revision as of 23:18, 5 July 2009
Getting Started
Before you start, you should be familiar with Bugzilla, which is where contributed code changes are submitted for review. You should ideally also have a working environment on a development system where you can test patches. See Dev Getting Started for more info.
When you're ready, this link will take you directly to the list of review requests in Bugzilla: "review?"
Basics of Patch Review
- Check for compliance with the DW style guidelines, as outlined on the Programming Guideline Checklist.
- Check for syntax errors if you're familiar enough with the language being used to spot them. Most DW pages are coded primarily in HTML/BML and Perl, but sometimes you'll run into JavaScript, CSS, or the particular markup used by the S2 system.
- Apply the patch in your development environment and check to see that it performs as advertised. See how to apply a patch if this is new to you. If the patch won't apply without errors, that's usually an indication that something else has changed in the relevant code since the patch was written, and you can either track down the changes yourself or ask the author to regenerate the patch.
Providing Feedback
If the code you've reviewed compiles, works, and conforms to the style guidelines, you should give it your seal of approval.
The way to do this is to click on the "Details" link next to the patch listing on the bug page, and in the popup menu next to the review heading, change the '?' to a '+'. Optionally, you can leave a comment telling the author what great work they've done.
On the other hand, if you've found a problem with the code, you should change the '?' to a '-' and provide the author details of whatever needs to be fixed.
Once a patch has been given the thumbs up by a reviewer, it should be committed by one of the developers with commit privileges, as outlined in the Dev Committing Guidelines. Make sure the commit flag on the patch is set to '?' so that it shows up in the queue of commit requests, unless indicated otherwise.
