<div dir="ltr">I would suggest doing a post-release day (branch cuts happen on Tuesday, so maybe use Wed or Thu) so that if we merge a lot of patches we'll have as much time as possible until the next release cut to test stuff on the beta cluster.<div><br></div><div>This is a great idea, and we need to think of continuous efforts to educate wmf developers about better code review practices.</div><div><br></div><div>I recommend reading this email Sam (phuedx) sent in June to mobile-l: <a href="https://lists.wikimedia.org/pipermail/mobile-l/2015-June/009357.html">https://lists.wikimedia.org/pipermail/mobile-l/2015-June/009357.html</a></div><div><br></div><div>And watch the talk if you are interested.</div><div><br></div><div><blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex" class="gmail_quote"><span style="color:rgb(0,0,0);font-family:'Latin Modern Roman';font-size:medium">Hey y'all,</span><br>I watch a lot of talks in my downtime. I even post the ones I like to a Tumblr… sometimes [0]. I felt like sharing Derek Prior's "Implementing a Strong Code Review Culture" from RailsConf 2015 in particular because it's relevant to the conversations that the Reading Web team are having around process and quality. You can watch the talk on YouTube [1] and, if you're keen, you can read the paper that's referenced over at Microsoft Research [2].<br>I particularly like the challenge of providing two paragraphs of context in a commit message – to introduce the problem and your solution – and trying to overcome negativity bias in written communication* by offering compliments whenever possible and asking, not telling, while providing critical feedback.<br>I hope you enjoy the talk as much as I did.<br>–Sam<br>[0] <a href="http://sometalks.tumblr.com/" target="_blank">http://sometalks.tumblr.com/<br></a>[1] <a href="https://www.youtube.com/watch?v=PJjmw9TRB7s" target="_blank">https://www.youtube.com/watch?v=PJjmw9TRB7s<br></a>[2] <a href="http://research.microsoft.com/apps/pubs/default.aspx?id=180283" target="_blank">http://research.microsoft.com/apps/pubs/default.aspx?id=180283</a><br>* The speaker said "research has shown" but I didn't see a citation<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> </blockquote><blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex" class="gmail_quote"><b>Notes (width added <i>emphasis</i>)</b><ul><li>Code review isn't for catching bugs</li></ul><ul><li>"Expectations, Outcomes, and Challenges of Modern Code Review"</li></ul><ul><li>Chief benefits of code review:</li></ul><ul><li>Knowledge transfer</li></ul><ul><li>Increased team awareness</li></ul><ul><li>Finding alternative solutions</li></ul><ul><li>Code review is "the discipline of explaining your code to your peers"</li></ul><ul><li>Process is more important than the result</li></ul><ul><li>Goes on to define code review as "the discipline of discussing your code with your peers"</li></ul><ul><li>If we get better at code review, then we'll get better at communicating technically as a team</li></ul>Rules of Engagement<ul><li>As an author, provide context</li></ul><ul><li>"If content is king, then context is God"</li></ul><ul><li><b>In a pull request (patch set) the code is the content and the commit message is the context</b></li></ul><ul><li>Provide sufficient context - bring the reviewer up to speed with what you've been doing in the past X hours</li></ul><ul><li><b>Challenge: provide at least two paragraphs of context in your commit message</b></li></ul><ul><li>This additional context lives on in the commit history whereas links to issue trackers might not</li></ul><ul><li>As a reviewer, ask questions rather than making demands</li></ul><ul><li>Research has shown that there's a negativity bias in written communication. <b>Offer compliments whenever you can</b></li></ul><ul><li><b>When you need to provide critical feedback, ask, don't tell</b>, e.g. "extract a service to reduce some of this duplication" could be formulated as "what do you think about extracting a service to reduce some of this duplication?"</li></ul><ul><li>"Did you consider?", "can you clarify?"</li></ul><ul><li>"Why didn't you just..." is framed negatively and includes the word just</li></ul><ul><li>Use the Socratic method: asking and answering questions to stimulate critical thinking and to illuminate ideas</li></ul>Insist on high quality reviews, but agree to disagree<ul><li>Conflict is good. <b>Conflict drives a higher standard of coding provided there's healthy debate</b></li></ul><ul><li>Everyone has a minimum bar to entry for quality. Once that bar is met, then everything else is a trade-off</li></ul><ul><li>Reasonable people disagree all the time</li></ul><ul><li>Review what's important to you</li></ul><ul><li>SRP (Single Responsibility Principle) (the S from SOLID)</li></ul><ul><li>Naming</li></ul><ul><li>Complexity</li></ul><ul><li>Test Coverage</li></ul><ul><li>... (whatever else you're comfortable in giving feedback on)</li></ul><ul style="color:rgb(0,0,0);font-family:'Latin Modern Roman';font-size:medium"><li>What about style?</li></ul><ul><li>Style is important</li></ul><ul><li>"People who received style comments on their code perceived that review negatively"</li></ul><ul><li>Adopt a styleguide</li></ul><br>Benefits of a Strong Code Review Culture<ul><li>Better code</li></ul><ul><li>Better developers through constant knowledge transfer</li></ul><ul><li>Team ownership of code, which leads to fewer silos</li></ul><ul><li>Healthy debate</li></ul></blockquote><div style="color:rgb(0,0,0);font-family:'Latin Modern Roman';font-size:medium"><div><br></div></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 5, 2015 at 1:58 AM, Arthur Richards <span dir="ltr"><<a href="mailto:arichards@wikimedia.org" target="_blank">arichards@wikimedia.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Generally looks good to me.<div><br></div><div>Couple of questions - are there plans to document/track 'unmaintained' repositories? Further, are there additional metrics that can come out of the GCD that we don't get out of Korma that would be useful information to have (eg patches to non-WMF maintained repos vs patches to WMF maintained repos)?</div></div><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Tue, Aug 4, 2015 at 1:45 PM, Quim Gil <span dir="ltr"><<a href="mailto:qgil@wikimedia.org" target="_blank">qgil@wikimedia.org</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Aug 4, 2015 at 10:21 PM, David Strine <span dir="ltr"><<a href="mailto:dstrine@wikimedia.org" target="_blank">dstrine@wikimedia.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>How do we come to consensus on the proposed date?</div></div></blockquote><div><br></div></span><div>There is no day of the week where Asia, Europe, and America can work 8 hours together comfortably, so we need to think on an activity that makes sense for a "Day" considering multiple timezones.</div><div><br></div><div>I think Release Management and Engineering managers will prefer to stop regular activities to focus on code review more on a Friday than any other day, with release trains and the big bunch of regular team meetings populating schedules. This is why I support a Friday, but will support whatever alternative day of the week makes happy to Engineering teams.</div><div><br></div><div>Again, it is an experiment, so we can pick a day and then analyze. We won't find the day that works well for everybody. :)</div></div><span><font color="#888888"><div><br></div>-- <br><div>Quim Gil<br>Engineering Community Manager @ Wikimedia Foundation<br><a href="http://www.mediawiki.org/wiki/User:Qgil" target="_blank">http://www.mediawiki.org/wiki/User:Qgil</a></div>
</font></span></div></div>
<br></div></div><span class="">_______________________________________________<br>
teampractices mailing list<br>
<a href="mailto:teampractices@lists.wikimedia.org" target="_blank">teampractices@lists.wikimedia.org</a><br>
<a href="https://lists.wikimedia.org/mailman/listinfo/teampractices" rel="noreferrer" target="_blank">https://lists.wikimedia.org/mailman/listinfo/teampractices</a><br>
<br></span></blockquote></div><span class="HOEnZb"><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div><div dir="ltr">Arthur Richards<div>Team Practices Manager</div><div>[[User:Awjrichards]]</div><div>IRC: awjr</div><div><a href="tel:%2B1-415-839-6885%20x6687" value="+14158396885" target="_blank">+1-415-839-6885 x6687</a></div></div></div>
</font></span></div>
<br>_______________________________________________<br>
teampractices mailing list<br>
<a href="mailto:teampractices@lists.wikimedia.org">teampractices@lists.wikimedia.org</a><br>
<a href="https://lists.wikimedia.org/mailman/listinfo/teampractices" rel="noreferrer" target="_blank">https://lists.wikimedia.org/mailman/listinfo/teampractices</a><br>
<br></blockquote></div><br></div>