[Numpy-discussion] let's use patch review

Ondrej Certik ondrej at certik.cz
Wed May 14 17:06:31 EDT 2008


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.

I think that a solution is a patch review. Every big/succesful project
does it. And the workflow as I see it is this:

1) Travis will fix a bug, and submit it to a patch review. If he is
busy, that's the only thing he will do
2) Someone else reviews it. Stefan will be the one who will always
point out missing tests.
3) There needs to be a common consensus that the patch is ok to go in.
4) when the patch is reviewed and ok to go in, anyone with a commit
access will commit it.

I think it's as simple as that.

Sometimes no one has enought time to write a proper test, yet someone
has a free minute to fix a bug. Then I think it's ok to put the code
in, as I think it's good to fix a bug now. However,
the issue is definitely not closed and the bug is not fixed (!) until
someone writes a proper test. I.e. putting code in that is not tested,
however it doesn't break things, is imho ok, as it will not hurt
anyone and it will temporarily fix a bug (but of course the code will
be broken at some point in the future, if there is no test for it).

Now, the problem is that all patch review tools sucks in some way.
Currently the most promissing is the one from Guido here:

http://code.google.com/p/rietveld/

it's opensource, you can run it on your server, or use it online here:

http://codereview.appspot.com/

I suggest you to read the docs how to use it, I am still learning it.
Also it works fine for svn, but not for Mercurial, so we are not using
it in SymPy yet.

So to also do some work besides just talk, I started with this issue:

http://projects.scipy.org/scipy/numpy/ticket/788

and submitted the code (not my code though:) in there for a review here:

http://codereview.appspot.com/953

and added some comments. So what do you think?

Ondrej

P.S. efiring, my comments are real questions to your patch. :)



More information about the NumPy-Discussion mailing list