[Numpy-discussion] let's use patch review

[Francesc Alted]

A Wednesday 14 May 2008, Ondrej Certik escrigué:
> Hi,
>
> I read the recent flamebate about unittests, formal procedures for a
> commit etc. and it was amusing. :)
> I think Stefan is right about the unit tests. I also think that
> Travis is right that there is no formal procedure that can assure
> what we want.
[snip]

For what is worth, Ivan and me were using patch peer review for more 
than a year in the PyTables project, and, although I was quite 
reluctant to adopt it at the begining, the reality is that it resulted 
in *much* better code quality to be added to the repository.

Here it is the strategy ww used:

0. A ticket is opened explaining the feature to add or the thing to fix.

1. The ticket taker (i.e. the responsible of adding the new code) 
creates a new branch (properly labeled) for the desired 
modification/patch.  The ticket is updated with a link to the new 
branch and the ownership of the ticket is transferred to the taker.

2. Once the modification is in the new branch, the ticket is updated 
explaining the actions done, and the ownership of the ticket 
transferred to the reviewer.

3. The peer reviews the code, and write suggestions in the same ticket 
about the new code.  If the reviewer doesn't feel the need to suggest 
anything, he will write this in the ticket also.  The ticket is 
transferred to the original author.

4. The original author should revise the suggestions of his peer, and if 
more actions are needed, he should address them.  After this, he will 
transfer the ticket to the peer for a new review.

5. Phase 3 and 4 are repeated until an agreement is held by both parts, 
and the discussion remains in the ticket (one can temporarily tranfer 
part of the ticket discussion to the mailing list, if necessary).

6. After the agreement, the original author commits the patch in the 
temporary code branch to the affected branches (normally trunk and the 
stable branch of the project) and removes the temporary branch.  The 
author has to tell explicitely in the ticket to which branches he has 
applied the new patch.

7. The ticket is closed.

Of course, this works great with a pair programming paradigm (as was the 
case for PyTables), but for a project as NumPy there are more 
developers than just a pair, so you should decide how to choose the 
reviewer.  One possibility is to form pairs by affinity, so that they 
can act normally together.  Another possibility would be to force all 
the developers to subscribe to the ticket mailing list, and, for each 
ticket that requires a peer review, a call should be sent in order to 
gather a reviewer (who can offer as a volunteer by adding a note to the 
ticket, for example).

I don't need to say that this procedure was not used for small or 
trivial changes (that were fixed directly), but only when the issue was 
important enough to deserve the attention of the mate.

My two cents,

-- 
Francesc Alted