On Sat, 2007-11-10 at 20:49 -0600, Jeremy Dunck wrote: > "I've written before on mailing lists that only about two out of every > five submitted patches I review go in unchanged on a good day and that > seems to match other maintainers' experiences, too" > -- > http://www.pointy-stick.com/blog/2007/11/02/development-experiences-version-control/
Well, that'll teach me to write stuff in public. :-) > > This obviously slows down patch inclusion. I was wondering if it > really takes a committer to polish the patch well enough for > inclusion? We all know code review is generally worthwhile, and I > regularly make improvements to my quite-good coworkers' commits on our > internal tree. > > I think Mozilla has this sort of thing in its "review, super-review" > process. It seems worth experimenting to see whether it just takes a > second set of eyes, not necessarily committer's eyes. My initial hope was that this would be part of the process before moving to "ready to review".That's the second set of eyes right there and we generally picked triagers initially who were showing signs of having good "feelings" And things are getting a lot better over time. Sometimes triagers are a little keen to mark their own patches as "ready for checkin", but again, it's never gotten out of hand or been a consistent problem. What Mozilla have done with reviewers and super-reviewers is instigated a fairly formal system whereby reviewers get some training in a sense and are required to be very good at it. I'm not sure we need quite that level at the moment. Django is still a small project. Historically, we close between 8 and 9 out of every 10 tickets opened, so our closure rate isn't bad (something I don't think is appreciated widely). There's nothing stopping triagers right now -- those who move things to "ready to checkin" from trying to match things as tightly as possible to what the final commit might look like. But I don't know how easy that will be, because a lot of it is opinion and feeling and that's hard to convey and document. > If we could get the ratio up to 4 of 5, would that make a significant > difference in inclusion speed, or is it still the committer's time to > review that slows it down? How many hours in a week (month?) do > committers spend on patch polishing? I'm not into counting hours much, since that would be rather depressing, I suspect. It's not wasted time, though, it's necessary time spent. Making small changes isn't the time consuming bit -- the review is. Making large changes means I'm often going to do a large rewrite of the whole thing (hardly unheard of), or bounce it back to the submitter. I'm trying to favour the latter for not-high-priority things these days, as a practical matter, which is certainly what Mozilla does. Remember, too, that some of these changes are because the code around the patch might have changed and it's not really reasonable to ask people to continually update their patches every month or so, if it's not a high-priority thing for us. I really don't mind those sort of changes, since I have to understand all the code anyway. What does take time is explaining all the changes and why I (or somebody else) might think they're necessary. Also, explaining that there's not necessarily a unique way to do things. During the last sprint, effbot did a fantastic job of dropping those sorts of notes into various tickets as he reviewed, but it's time consuming, so I personally probably don't spend as much time as I could giving that sort of direct feedback. I feel my time is better spent actually landing the code and hoping people are conscientious enough to read the commit when it lands if they want to see where we differ in opinion. I dunno ... gut feeling is we need more layers here, we already have triagers who are volunteering and doing a pretty good job. It's frankly fairly surprising how similar Russell, Adrian, Jacob, Gary and I are in our thinking on some things. Teaching how to do that so people can write a patch that requires no changes is pretty tough. A lot of it just experience, though. Trying to understand why something was changed (or "why possibly", since there are often multiple right answers). Hmm ... a longer answer than I intended, but hopefully the conclusion is clear. Review takes time, but I don't think we're approaching it the wrong way on a fundamental level. Regards, Malcolm -- Remember that you are unique. Just like everyone else. http://www.pointy-stick.com/blog/ --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~----------~----~----~----~------~----~------~--~---
