Peer reviewing: Difference between revisions
From MoodleDocs
Jason Fowler (talk | contribs) |
Jason Fowler (talk | contribs) (→GIT) |
||
Line 51: | Line 51: | ||
===GIT=== | ===GIT=== | ||
Ensure that: | |||
* The commit message includes the tracker issue number and the component. Ideal format is <code>MDL-xxxx Component Commit message</code> | * The commit message includes the tracker issue number and the component. Ideal format is <code>MDL-xxxx Component Commit message</code> | ||
* | * The GIT history is clean and the work has been rebased in to as few commits as is necessary | ||
* | * The original author of the work has been given credit within the commit (as author of in the commit message if changes were made) | ||
==See Also== | ==See Also== | ||
* [http://moodle.org/plugins/view.php?plugin=local_codechecker Code checker plugin] | * [http://moodle.org/plugins/view.php?plugin=local_codechecker Code checker plugin] |
Revision as of 07:21, 10 February 2012
Peer review check-list
These are points to consider while peer-reviewing issues. They can also be applied when solving issues, to reduce the likelihood of problems during peer-review and integration.
Syntax
Ensure that:
- there are no unnecessary blank lines in the new code;
- blank lines do not contain spaces;
- variables are named correctly (all lower case, no underscores);
- functions are named correctly; and
- there are no unnecessary changes to whitespace in other areas on the file.
- new strings Don't Use Camel Case
See the Coding style guide for details.
Output
Ensure that:
- the code doesn't use buffered output unless absolutely necessary; and
- lang-strings are used instead of hard-coded strings for text output.
Databases
If there is SQL code you can test quickly, do so.
Ensure that:
- there are minimal DB calls (no excessive use of the DB); and
- the code uses SQL compatible with all the supported DB engines.
Misc
Ensure that:
- the code seems to solve the described problem;
- there are specific testing instructions that state how, as well as what, to test;
- the code doesn't use deprecated functions (see Deprecated functions in 2.0);
- the code makes sense in the broader scheme of things (look at the whole function, not just the altered code); and
- the code is easy to understand, and where it isn't, comments have been provided.
- It doesn't break XHTML
- It doesn't remove strings, or rename strings from stable branches (permitted only in master)
- php docs have been updated and adhere to coding style guide
- The idea/solution makes sense and is complete.
- The developer has searched for and fixed other areas that may also have been affected (provided they are in the scope of the original issue)
- String changes include AMOS syntax where required (moving, copying, deleting strings)
- No inline styles have been used in HTML output (everything has to be in CSS)
- CSS has been added to the appropriate CSS files (base, specific area, sometimes canvas)
- $_GET, $_POST, $_REQUEST, $_COOKIE, and $_SESSION are never used
- Unit tests all pass for related areas where changes have been made
- Uses sesskey for all write actions where appropriate (some read actions as well although not as common)
Tracker
Ensure that:
- Appropriate labels are added if needed (see Tracker Issue Labels)
- Testing instructions are present and sufficient for a developer to follow
GIT
Ensure that:
- The commit message includes the tracker issue number and the component. Ideal format is
MDL-xxxx Component Commit message
- The GIT history is clean and the work has been rebased in to as few commits as is necessary
- The original author of the work has been given credit within the commit (as author of in the commit message if changes were made)