<div dir="ltr">Hi,<div><br></div><div>In general, given the griping I've seen over time for how hard it is to do code review on GitHub Pull Requests, I think we (the open source community) owe it to GitHub to try out the new feature they rolled out to address and give them feedback--that's the only way the feature can ever meet our needs.</div><div><br></div><div>For our (matplotlib) case, I think we get a few good gains:</div><div>- line comments are batched so we can cut down on the number of notifications</div><div>- Your work is saved so you can come back later and finish--and adjust when you see something later that invalidates your comment *before* commenting for all to see</div><div>- Reviews are atomic items, so you can get an idea of what a specific reviewer thinks (and not lose pieces)</div><div>- By having the state of Approved/Needs Changes actually tracked, there's no need to read through the entire PR to see what objections are left. (Granted, it does mean people have to come back and approve it once the changes are made)</div><div><br></div><div>Seems like a lot to gain for little to no extra effort--having used it, I certainly fail to see any problem. The matter of turning on required reviews for protected branches is a whole other matter, though I will say you can always let admins override.</div><div><br></div><div>Ryan</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Sep 19, 2016 at 8:24 PM, Thomas Caswell <span dir="ltr"><<a href="mailto:tcaswell@gmail.com" target="_blank">tcaswell@gmail.com</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">Folks,<div><br></div><div>Have people had a chance to look at the new review tools on github?  Do we want to work these into our workflow?  It looks like we can set it so that PRs can only be merged via the web UI with at least one approve review and no 'needs work' reviews if we want.   My initial reaction is we should try to use the review tools, but hold on on the engineering control on merges until we have a better sense of what that would look like.</div><div><br></div><div>Has anyone looked at the 'projects' feature yet?</div><div><br></div><div><div>A suggestion from Nelle Varoquaux is to follow skimage/sklearn and on review add a [mrg+N] to the title to help other devs find PRs that are almost ready to merge, but need another set of eyes.  </div></div><div><br></div><div>Tom</div></div>
<br>______________________________<wbr>_________________<br>
Matplotlib-devel mailing list<br>
<a href="mailto:Matplotlib-devel@python.org">Matplotlib-devel@python.org</a><br>
<a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" rel="noreferrer" target="_blank">https://mail.python.org/<wbr>mailman/listinfo/matplotlib-<wbr>devel</a><br>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div>Ryan May<br><br></div></div></div>
</div>