Hi all, 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. 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 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, however I recognise the name of the submitter. 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. 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. I feel a lot more qualified to evaluate code quality than desirability of function. 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. 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. Regards, -Tennessee -- -------------------------------------------------- Tennessee Leeuwenburg http://myownhat.blogspot.com/ "Don't believe everything you think"
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:
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
Martin v. Löwis wrote:
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.
Both may be needed, depending on the issue. But see below.
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:
Briefly, as I read it: Raymond proposed that his named tuples be used with Barry and Skip's csv module. He got a short reply from Skip but went on to other things. A year later, two new people entered and energized the discussion, Barry said 'useful', Skip commented on the details. The second half of the discussion has been thrashing out and getting consesus on the devilish details. What is devilish is that the merits of feature details sometimes interacts with code quality. This is pretty typical. It looks to me like this will be in 3.1.
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)
Every Friday a list of opened and closed issues is posted. Even if you have nothing to say, you can add yourself to the nosy list.
I feel a lot more qualified to evaluate code quality than desirability of function.
Good, in the sense that I believe the former is rarer and more needed. There are 5 broad areas covered in the tracker: 1. C code, interpreter core 2. C code, extension modules 3. Python code, modules 4. Tests of the above 5. Documentation of above Where to start? What interests you most? Or look at the thread "draft 3.1 release schedule" and posts giving features aimed at 3.1. For instance, http://bugs.python.org/issue5237 has a new C patch that could use at least a preliminary eyeball, pending the more polished patch. Help adding the test patches would be welcome by me and, I am sure, by Eric. Terry Jan Reedy
On Mon, Mar 9, 2009 at 20:55, Terry Reedy <tjreedy@udel.edu> wrote:
Martin v. Löwis wrote:
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.
Both may be needed, depending on the issue. But see below.
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:
Briefly, as I read it: Raymond proposed that his named tuples be used with Barry and Skip's csv module. He got a short reply from Skip but went on to other things. A year later, two new people entered and energized the discussion, Barry said 'useful', Skip commented on the details. The second half of the discussion has been thrashing out and getting consesus on the devilish details. What is devilish is that the merits of feature details sometimes interacts with code quality. This is pretty typical. It looks to me like this will be in 3.1.
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)
Every Friday a list of opened and closed issues is posted. Even if you have nothing to say, you can add yourself to the nosy list.
I feel a lot more qualified to evaluate code quality than desirability of function.
Good, in the sense that I believe the former is rarer and more needed.
There are 5 broad areas covered in the tracker: 1. C code, interpreter core 2. C code, extension modules 3. Python code, modules 4. Tests of the above 5. Documentation of above
This is somewhat covered by components, but it's implicit. Would it be worth making this explicit? I have always wondered if people would be more willing to help out if they could easily search for pure Python code issues if that is as far as they feel comfortable. We could then have the components merge into keywords or just take out the implicit type of issue part. Or we just don't worry about auto-assignment. I think Georg is the only one getting auto-assignment and I think we all know to assign him doc stuff. =) -Brett
Brett Cannon wrote:
This is somewhat covered by components, but it's implicit. Would it be worth making this explicit? I have always wondered if people would be more willing to help out if they could easily search for pure Python code issues if that is as far as they feel comfortable.
If and when I am ready to move from working on documentation issues (which seem to becoming fewer as the 3.x transition is completed) to code issues, that would be helpful. What would be really helpful is to have library issues tagged and sorted by specific modules (or modules, if more than one), but I do not know how that might be done.
On Tue, Mar 10, 2009 at 11:43, Terry Reedy <tjreedy@udel.edu> wrote:
Brett Cannon wrote:
This is somewhat covered by components, but it's implicit. Would it be worth making this explicit? I have always wondered if people would be more willing to help out if they could easily search for pure Python code issues if that is as far as they feel comfortable.
If and when I am ready to move from working on documentation issues (which seem to becoming fewer as the 3.x transition is completed) to code issues, that would be helpful. What would be really helpful is to have library issues tagged and sorted by specific modules (or modules, if more than one), but I do not know how that might be done.
It would have to be a text field that people just filled in. Making a list that had to be kept up-to-date would be a disaster. But one would hope that simply searching for an issue with a specific module name would be enough to warrant not having to have the field. -Brett
participants (4)
-
"Martin v. Löwis" -
Brett Cannon -
Tennessee Leeuwenburg -
Terry Reedy