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 _______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
