I am glad to see that we are doing so well with reviews, but there is one crucial piece of this process that in my opinion is still too often missing. The piece is the history about the review comments and decisions made in reviews.
The reviews happen, we just need to document what was said in the review. Consider this example case: You are reading some code that was written a year ago by someone other than you. You understand the code, but you realize you would not have done it exactly in the way it is implemented. There are no comments that explain why it was done that way and not your way. The checkin comment does not say that either. You go visit the bug and see that there were two patches - first one written the way you would have done it, and a second one that received a r=someone that is the checked in version. But no mention why this is. Now what do you do? Maybe the easiest way is to just ask either the committer or the reviewer. But they might not remember - after all it was a year ago. And if both of those persons have since left the project with no contact information, you can't even ask. You could try asking on dev, but maybe nobody else knows either. You'd then have to try out the different ways and maybe analyze their code to figure out why it is the way it is. All in all, you've probably wasted from few minutes to several hours of time on this. But what if, when you'd gone to the bug, you'd seen this: (review comments of first patch) Please change thisfunnyfunchere() to thisotherfunc() because the first one misses an edge case relevant in this context when ... blah blah blah... >From personal experience I find these extremely useful for several reasons. The first is an educational one: when I see someone's detailed review commentary I learn new things. Second is that when I try to understand a piece of code it helps me when I see the reasoning for why it is written the way it is. It also helps me avoid introducing bugs when I have to modify that code. -- Heikki Toivonen
signature.asc
Description: OpenPGP digital signature
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ Open Source Applications Foundation "Dev" mailing list http://lists.osafoundation.org/mailman/listinfo/dev
