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 and deployers, etc.

The process is composed by the following phases, explained below: preliminary discussion, proposal, discussion and acceptance.

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 accepted.

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
• 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

Discussion

Maintainers and peers of the Sugar modules periodically check the review queue in Trac. If a few day passes 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, or to 'r-' if further information or action is requested from the submitter.

Note that if your patch is marked with 'r-' it doesn't mean that your code or ideas are bad, just that the maintainer is still not convinced that the patch can be accepted in its current state. 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

Process guidelines

Break your projects into small pieces that can be submitted for review independently. Submit for review whenever you think you need a review - you can tell that best. The reviewer will use review+ or r+ whenever the patch is accepted and ready to go into git master.

The workflow should be something like:

• Code a small part
• Submit to review
• Push to master
• Repeat

There will be cases when this will not be possible but we want to try to follow this guidelines.

We could consider that 1500 lines starts to be too much for a diff, although factors like the add/delete ratio or the invasiveness of the changes should be taken in account.

Patch guidelines

Please try to use pylint to verify your patch for things like exceeding 80 columns etc, unused imports and unused variables. Pylint is not a tool you can rely on 100% but it helps to follow some guidelines and to avoid the most stupid errors like typos. sugar-jhbuild contains a pylintrc you can use in the scripts/data directory: pylintrc (follow "raw blob data")

pep8.py catches more style errors than pylint, so make sure to run that one, too.

In the sugar packages use 'make distcheck' to make sure all files are included and the POTFILES.in is up to date.

Create a patch

Normally you can get a patch from git with:

git diff

If you already committed part or all of the new code you need to specify the id of the last commit before yours:

git diff 34a4876f93894309f77b00281b5cb1bb72b3a1e4

Or if the commit you want the diff of was the most-recent commit, you can do:

git diff HEAD^

Another alternative that will give you a patch in a file called 0001-My-cool-patch.patch:

git add new_file1 new_file2
git commit -a -m 'My cool patch'
git format-patch HEAD^
git reset --hard HEAD^

Reviewer guidelines

• For complex patches you might want to start with an overall design of the patch.
• Make sure the submitter provided a testcase before approving the patch.
• Make sure that API policy rules are respected.
• Keep an eye on readability, we are roughly following Guido's PEP 8
• Change the r? keyword to r- to indicate that the patch needs work.
• Change the r? keyword to r+ if the patch is ok to be pushed.

Push the patch

If you have write access to the sugar repository you can just do it yourself. 

If you don't have write access to the repository commit the patch to your local repository.

git commit

Generate a patch set using git. The following command will create one file per commit in the current directory.

git format-patch origin

Make sure that the ticket number is referenced in the commit description!

Buildbot

We are running periodic builds of Sugar using Buildbot. If you break the build you are responsible to get it fixed. If you don't, the release team will take care of it, most likely by reverting your patch. More in detail:

• Every change that causes build or check failures should be immediately fixed or reverted.
• Every change that causes check warnings should be fixed or reverted within 24 hours.

You can use the check command in jhbuild to execute the same checks that Buildbot is doing. You can use the jhbuild pylint configuration, to lint the files your patch modifies.

Remind the maintainers about patches

If you don't get any response from the maintainers about the patches you submitted in a few days, feel free to remind about them using the Sugar mailing list or IRC channel.