-
Notifications
You must be signed in to change notification settings - Fork 1
Check list for peer reviewing patches and pull requests
-
Contributions to our plugins from within and from outside the german-speaking higher education Moodle community are highly appreciated. We always appreciate if developers take the time to improve and extend our plugins in any way.
-
Having said that, we have to highlight that our plugins are built with passion and contributions to the plugins should strive to match this quality level as well.
-
Every community member, regardless if he has push rights to the affected plugin repository or not, is invited to do a peer review of a submitted patch or pull request. To help with this task, we built the following checklist.
-
Is the commit message understandable? There is no real need (but highly appreciated) to summarize the whole patch in some paragraphs within the commit message. But it is mandatory that the first line of the commit message is understandable and self-explaining so that everyone gets the idea what the patch is about. Ideally, the commit message also references the Github issue which is solved by the patch.
-
Is the patch author correct? Generally, it is highly encouraged that the author of the patch maps to a real human and an existing e-mail address. Commit authors like
root <root@localhost>
are not acceptable. -
Is there a new line in CHANGES.md? Generally, a patch should include a new line in CHANGES.md which contains the commit message as well. This is especially helpful as the CHANGES.md file's content is automatically published on the moodle.org/plugins repository when a new version is released there.
-
Is there new information in README.md? Generally, if there were major changes or new functionalities, changes to README.md might be recommended or necessary.
-
Does the code contain enough comments? There is no mandatory code-to-comment ratio for our plugins, but generally the purpose of the plugin code should be understandable by reading just the comment lines and skimming over the PHP / Javascript lines.
-
Are the PHPDoc comments correct? Generally, there is no real need to document everything with PHPDoc in our plugins. However, at least the minimum documentation requirements which are also demanded by the Moodle PHPDoc checker (which runs within Github actions, see below) have to be fulfilled.
-
Is the
$plugin->version
value increased? Not every patch needs to increase the version in version.php. But there are clear situations, especially if new settings are introduced to settings.php, new strings are added to the language pack, new classes were introduced, new CSS styles or JavaScipt code was introduced when a version increase is recommended or even mandatory. -
Is the
$plugin->release
value kept untouched? Generally, the release string should only be changed when an official release is made and published to the moodle.org/plugins repository.
- Does lib.php only contain necessary functions? The lib.php file of a plugin is loaded more or less at every page load, regardless if the plugin is needed on this page or not. This is because lib.php can contain hook functions. Against this background, functions which help the plugin to realize its own logic, should not be placed in lib.php and should be placed in a locallib.php or autoloaded class instead.
-
Are there any non-english strings in the english language pack or are the any non-english language packs? All translations are done on AMOS which means that there must not be any non-english strings in the patch.
-
Are magic strings added? There are some assets in a plugin which have to be accompanied by a magic string in the language pack, for example new capabilities, new scheduled tasks or new events. If any of these assets were added, the magic string should be added to the language pack as well.
- Many of our plugins have a good coverage with Behat tests, some are covered with PHPUnit tests as well. As a general rule, patches must not break existing tests which were running successfully before. Additionally, patches which introduce new functionality should contain new automated tests which cover the new functionality as well.
- Is the Github action run all green? With every patch and every pull request, a Github action run is triggered which checks especially the plugin code against the Moodle coding styles and runs all automated tests. If this Github action run is not all green, the patch must not be merged and the issues have to be fixed first.
-
Does the patch consist of one single commit? One contribution to a plugin should consist of one commit. If there are multiple commits, these should be squashed during the merge.
-
Is the patch really focused on one single purpose? It might ok if a patch changes some minor things in the surroundings of the patched lines if this helps to improve the overall quality of code. However, if a patch contains completely unrelated changes like fixing typos in lines which would not be touched by the patch otherwise, these changes should be split in separate patches.
-
Are the any surplus files in the patch? If the commit author added any file by mistake or by purpose which does not relate to the patch's purpose, especially Mac OS
.DS_Store
files or any IDE management files, these files should be dropped before merging the patch.