[Python-Dev] reviewing patches

["Martin v. Löwis"]

> I have seen it said that one very useful activity is reviewing patches.
> Of the issues in the tracker, it is not immediately clear to me what is
> required of such a review. Many of these patches appear to be bundled in
> with feature requests, leaving the question of whether the review it
> judging the quality of the code or the merits of the feature request.

That's correct.

> I
> do realise that these issues have probably been previously discussed on
> this list. Let's take as an example the following issue:
> 
>   http://bugs.python.org/issue1818

This is somewhat of a special case, as the original submission is from
a committer (Raymond). If he would feel like it, he could commit it
at any time. That he didn't is probably because he is uncertain himself.

> This has obviously been around for a while, but not been implemented for
> some reason. There are no links in any of the comments to any email
> threads regarding its merits

This likely means there hasn't been any email communication. This is
common; often discussion is carried out entirely in the tracker (and
if you aren't on the nosy list of the issue, you will miss the
discussion completely)

> This makes me think the patch is probably implementing
> desirable functionality. However, it has no priority set, which makes me
> think that the community hasn't yet given it any kind of 'status',
> insofar as a priority can stand in for desirability.

No. It means that the classification fields are often completely
ignored, both by submitters and reviewers.

> It seems to make sense that code quality reviews should be separated
> from feature request reviews (quality code evaluation vs desirability of
> function evaluation) but I don't see how this occurs.

Most likely, the functionality itself is uncontested. This is typically
because
a) some reviewers agreed that the functionality is desirable, and
b) some reviewers didn't consider the possibility of questioning the
   functionality, and
c) some possible opponents are unaware of the discussion in the first
   place.

> I feel a lot more
> qualified to evaluate code quality than desirability of function.

I think it is fairly straight-forward to ask that you can access the
fields of a CSV list by field name, rather than by index; I would take
that as granted.

Notice that much of the discussion is about fine points of the API,
which is an indication that the actual design of the functionality is
tricky, not just the implementation. It would be helpful to be a heavy
CSV user to understand all aspects, and to evaluate usefulness of a
specific API.

> Questions like this make it difficult for someone in my position, who is
> happy to tackle 'whatever needs to be done', to begin the task of patch
> reviews.

I recommend that you look at patches with no long discussions - things
that did not get any attention at all so far. Issues that have already
long discussions typically are inherently difficult, and its not always
clear that another reviewer will be able to progress the issue - unless
he happens to be an export on the matter, such as Alexander for knots
on ox carts.

> While I'm not sure that a formal or semi-formal approval
> process would make anything better, I think it would be good if there
> were some kind of 'executive review' process by which an issue could be
> marked as being a good thing or not.

If you think creating more keywords would help, please feel free to
propose some. If it is more status values, or more whatever - likewise.
All of these are cheap to create.

Regards,
Martin