[Python-Dev] how can I review? [was: Encouraging developers]

["Martin v. Löwis"]

Jim Jewett schrieb:
> The 5:1 patch review is a good idea -- but what is the procedure for
> reviewing a patch?
> 
> I often comment on patches.  Does this count as a review? 

Sure. Ideally, a review should bring the patch to an "accept-or-reject"
state, i.e. it should lead to a recommendation to the committer.
An explanation should include the reasoning done (eg. for accept:
useful feature, complete implementation, for reject: patch is 
incomplete, has undesirable side effects).

However, in that 5:1 deal, any kind of review was good in the past,
even though it doesn't lead to a decision.

> Would anyone know if it did?

I often see your reviews (thanks!), but I don't keep track.

> If I were going through five at the same time, and I had a sixth to
> push, I could post here.  Normally, I just make a comment on the SF
> tracker.  As far as I know, that makes zero difference to any
> committer (at least) until they have already decided to look at the
> issue themselves.  At best, I am shaving off a round of
> back-and-forth, if there would have been one.

Indeed. That is already useful, although it might help more if you
stated a recommendation (I know you do in many cases).

If you have a list of patches that you think are ready for
decision, please post the list here, listing clear accepts and
clear rejects.

For uncertain cases, you can try to start a discussion; make
your lay out pros and cons and explain what side you are leaning
to.

> Sometimes all I say is "What about case X"?  The patch isn't ready to
> be committed yet.  It might be that comments are enough, but it isn't
> ready.  I wouldn't want the fact fact that I commented to grab a
> committer's attention.

If you think the patch is not complete, state so: "It is not complete
because it doesn't support case X". If the submitter doesn't respond,
you can consider revising it yourself, if you think the patch is 
important, or recommend rejection (if you think it isn't that
relevant).

> Sometimes the patch is good, or they deal with all issues.[1]  At that
> point, I ... stop commenting.  I don't know of any way (except
> personal email) to indicate that it was reviewed, let alone approved.

For the record, state your position in a comment. It is important
to record what position people have on patches they reviewed.
If they merely review, and then don't communicate their findings,
it is wasted time.

> One option would be a designated wiki page listing who reviewed
> patches when and whether they are ready -- but it seems sort of
> heavyweight, like it ought to be part of the tracker.

I agree. Maybe we should add some "reviewed by" field to the roundup
tracker where reviewers can record that they brought the patch
to a state ready for committing/final rejection (I think a second
check is still needed, as the case of splitext shows: you were in
favor of rejecting it because of the incompatibility, but it looks
like the majority of users is in favor of accepting because they
consider the current behavior buggy).

> If some committers are interested (and tell me how they want the
> review notification), I would be happy to pre-filter some stdlib
> patches.  If there are several volunteers wanting to split the work, I
> would be willing to organize the split however the others prefer.

Not sure how you are organizing this work: If you have completed
a review (i.e. so that just some technical action would need to
be taken), feel free to post to python-dev (ideally with a
python.org/sf/number link so people can easily follow your
analysis), with a subject like "recommend for
rejection/acceptance".

The tricky ones are really the incomplete ones: if neither
the submitter nor you yourself feel like completing the
patch, posting to comp.lang.python might also reveal some
contributors.

Regards,
Martin