Development Team/Code Review
Introduction
Because Sugar is an open source project, anyone can contribute to it and thus improve the learning experience of children all around the world.
Once you have fixed a bug or implemented a new feature, you can send it to the current maintainers of Sugar by following the code review process.
This process allows the maintainers to feel confident about taking the responsibility of delivering your code to children. This means applying it to the current code base, fixing any bugs that are later discovered in your code or as consequences of it, modifying your code to gain new features, assisting packagers, deployers and translators, etc.
It is strongly recommended that you read about the review process and Code guidelines before starting to code.
The process is composed by the following phases, explained below: preliminary discussion, proposal, discussion and acceptance.
Phases
Preliminary discussion
Except when the patch will be trivial, it's a good idea to notify the maintainers that you are going to work on this issue and discuss possible approaches with them. This avoids duplicated work and will give you an idea of what is considered a good idea by the maintainers before you commit substantial work in the patch itself.
May be interesting as well to discuss with the broad community about the usefulness of any proposed new feature. This way the maintainers will feel more motivated to commit their own time so that your work gets included.
Plan your work together with the maintainer in such a way that code will be submitted in chunks of reasonable size, a good rule of thumb is in patches of below 2000 lines.
Proposal
- Create your patch using the 'git format-patch' command.
- If your patch addresses an issue which is not registered in the Sugar Labs trac instance, please open a new ticket for it.
- Attach the patch to the ticket
- Alternatively, you can add a link pointing to a cloned tree. Make sure it contains only the changes you propose for review.
- Add the keyword 'r?' to call the attention of reviewers.
- Add a testcase in a ticket comment. You need to mark it with |TestCase|. This adds better readability and our script that pulls together the test cases for each release is able to find it as well. For example:
|TestCase| Click on Browse, Read, Pippy icons in the homepage and make sure all of them starts correctly.
If your patch is a new feature and reasonably big, you may prefer to submit it for review to the Sugar-devel mailing list. If you do please add the 'r?' keyword to the ticket anyway and reference the mailing list thread.
In order to make it easy for the reviewer please:
- note which module is effected e.g. sugar, sugar-toolkit...
- note possible dependencies e.g. the patch is for sugar but depend on the current HEAD of sugar-toolkit which went in 5 seconds ago (see also separate section about external dependencies below)
Discussion
Maintainers and peers of the Sugar modules periodically check the review queue in Trac. If a few days pass without comment since the patch was submitted, please ping the maintainer in IRC or in the mailing list. At times maintainers are very busy and will appreciate the ping, even if repeated.
The reviewer will read your patch, evaluate its impact to the existing code base and comment on it. The 'r?' keyword will be changed to 'r+' if the patch is accepted, to 'r!' if further information or action is requested from the submitter, or to 'r-' if the patch gets rejected.
See Code guidelines for common criteria for code in Sugar.
Once you have answered the questions and/or attached a new patch, set the 'r!' keyword back to 'r?'.
Acceptance
Once your patch has received a 'r+', the reviewer will push your code to the repository. From now on your code is part of the Sugar code base and the community will maintain it for you.
Commit
If you have commit access to the repositories you should read the commit guidelines as well before submitting your changes.
Dependencies
Dependencies need to be fulfilled on all platforms running Sugar (i.e. they need to be available at all and need to be installed which means occupying disk space), so think twice before introducing new ones. Please discuss them on sugar-devel in advance and clearly state them in the ticket. It's a always a good idea to check whether sufficiently recent versions of your proposed dependencies are already packaged for all (stable releases of the) major distributions, on all of their (semi-)supported architectures (e.g. ARM and PowerPC). If not, start requesting them (at the distribution bug trackers) ASAP and also mention it on sugar-devel and the ticket.