[Numpy-discussion] let's use patch review

Ondrej Certik ondrej@certik...
Wed May 14 20:31:14 CDT 2008


On Thu, May 15, 2008 at 1:58 AM, Eric Firing <efiring@hawaii.edu> wrote:
> 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.

It's not me who should make this decision, as I have contributed in
total maybe 1 patch to numpy. I am just suggesting it as a
possibility.

The numpy developers are free to choose the workflow that suits them best.

But if you are asking for my own opinion -- yes, I think all code
should be reviewed, including core developers, that's for example what
Sage does (or what we do in SymPy), because that brings other people
to comment on it, to help find bugs, to get familiar with it and also
everyone involved learns something from it. Simply google why patch
review is useful to get arguments for doing it.

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

That everyone involved in the discussion agrees it should go in as it
is. I am sure you can recognize very easily if there is not a
concensus.

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

Those, who have commit access are definitely not anyone. Only those,
who have showed they can be trusted
not to break things.

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

Both is possible. Some projects require all patches to go through a
review and I personally think it's a good idea.

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

That's what the patch review is for -- people will comment on this and
if a consencus
is made that a test is not necessary, ok.

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

It seems only the owner of the issue (in this case me, because I
uploaded your code) can add new patches to that issue. So simply start
a new issue
and upload it there. If there were more revisions from you, it would
look like this:

http://codereview.appspot.com/970

Ondrej


More information about the Numpy-discussion mailing list