<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jul 1, 2011, at 6:57 PM, David Ripton wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; font-family: Menlo; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><span class="Apple-style-span" style="font-family: monospace; ">Working with patches because you don't have svn commit rights is<span class="Apple-converted-space"> </span>annoying, but this annoyance is a relatively minor fixed cost.</span></span></blockquote><div><br></div><div>It's still important for us to reduce this cost; even if it's not the bottleneck, we have to optimize first where we <i>can</i> optimize :).</div><br><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; font-family: Menlo; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><span class="Apple-style-span" style="font-family: monospace; ">The real<span class="Apple-converted-space"> </span>issue, for controversial features, is achieving consensus, and then<span class="Apple-converted-space"> </span>getting your feature in before consensus is lost.<br></span></span></blockquote></div><br><div>Yes, absolutely. And there's are some important guidelines for reviewers that can be inferred from this:</div><div><br></div><div><ol class="MailOutline"><li>Try to stick to coding-standard stuff as much as possible, especially if there's been a previous review. Don't bring up "I think it would be better if..." things, except to say "file an additional ticket".</li><li>If there's a previous review, as much as possible, stick to the points brought up in the previous review. Make sure they're addressed, and try not to add a pile of conflicting stylistic suggestions.</li><li>When you do a review, try to be as thorough as possible. Don't <b>ever</b> do a review that says "update @since markers" or "2 blank lines between methods" and nothing else; at least, you need to say "... and then it will be ready to merge". Remember that when you take it out of review, no other reviewer is going to look at it until the author fixes it and resubmits it, which may be quite a while. If you feel like adding some partial commentary to help the next full reviewer, just add a comment, don't remove the review keyword.</li><li>Be explicit about what happens next, even if it's going to be redundant. Always say "... and it will need another review" or "... and then merge". Try not to voice a vague dissatisfaction with the architecture of something without an explicit suggestion about (A) what should be done, and (B) whether it <i>needs</i> to be done before the feature can be merged.</li></ol><div><br></div><div>For contributors, one suggestion: make implementation details private as much as possible, so that the reviewer will have to consider the aesthetics of the implementation details less. The smaller the public API of the contribution, the easier it is to avoid bikeshedding around method names and class placement :).</div><div><br></div><div>None of this would have helped in particular on the IPv6 stuff, but given that that affected an extremely core API, and had a ton of fiddly little details, I'm not sure much <i>could</i> have helped on that one...</div><div><br></div><div>I know I've broken these rules myself on occasion, and I'd like to encourage other reviewers to call me out on it when they notice it :).</div></div><div><br></div><div>-glyph</div><div><br></div></body></html>