[Numpy-discussion] let's use patch review
Thu May 15 12:46:46 CDT 2008
A Wednesday 14 May 2008, Ondrej Certik escrigué:
> 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.
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,
More information about the Numpy-discussion