[Numpy-discussion] let's use patch review

[Eric Firing]

Ondrej Certik wrote:
> 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:

Are you sure numpy is big enough that a formal mechanism is needed--for 
everyone?  It makes good sense for my (rare) patches to be reviewed, but 
shouldn't some of the core developers be allowed to simply get on with 
it?  As it is, my patches can easily be reviewed because I don't have 
commit access.

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

That we can agree on!

> 3) There needs to be a common consensus that the patch is ok to go in.

What does that mean?  How does one know when there is a consensus?

> 4) when the patch is reviewed and ok to go in, anyone with a commit
> access will commit it.

But it has to be a specific person in each case, not "anyone".

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

How does that fit with the workflow above?  Does Travis commit the 
bugfix, or not?

> 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).
That is overstating the case; for 788, for example, no one in his right 
mind would undo the one-line correction that Travis made.  Chances are, 
there will be all sorts of breakage and foulups and the revelation of 
new bugs in the future--but not another instance that would be caught by 
the test for 788.
> 
[...]
> http://codereview.appspot.com/953
> 
> and added some comments. So what do you think?
Looks like it could be useful.  I replied to the comments. I haven't 
read the docs, and I don't know what the next step is when a revision of 
the patch is in order, as it is in this case.

Eric
> 
> Ondrej
> 
> P.S. efiring, my comments are real questions to your patch. :)
> _______________________________________________
> Numpy-discussion mailing list
> Numpy-discussion at scipy.org
> http://projects.scipy.org/mailman/listinfo/numpy-discussion