<div dir="auto"><div>Agreed. <br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jan 17, 2019, 12:25 PM David Stansby <<a href="mailto:dstansby@gmail.com">dstansby@gmail.com</a> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Apologies, my previous email was unnecessarily negative...</div><div><br></div><div>I think the new system is a good idea, so here's where we seem to be. In terms of process:</div><div><br></div><div><div>1) PR opened<br></div><div>2) PR gets 1 positive review</div><div>3) 2 weeks after review the PR gets <b>no other significant activity, </b>and <b>qualifies for simpler review</b><br></div><div>4) PR opener or reviewer 1 pings all Matplotlib devs<br></div><div>5) If 2 weeks after pinging no other significant activity has occurred, PR can be merged by PR opener or reviewer 1</div><div><br></div><div>Does everyone agree on this? The bits I've put in bold still need to be defined; I don't have time now, but can provide a summary of these later.</div><div><br></div><div>David<br></div></div><div><br></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="m_4924008530671322732gmail_attr">On Thu, 17 Jan 2019 at 00:48, Antony Lee <<a href="mailto:antony.lee@institutoptique.fr" target="_blank" rel="noreferrer">antony.lee@institutoptique.fr</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Wed, Jan 16, 2019 at 7:36 PM David Stansby <<a href="mailto:dstansby@gmail.com" target="_blank" rel="noreferrer">dstansby@gmail.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>I think real improvements not getting in at the rate they are submitted is just how it works; if I started submitting a PR a day for the next 50 days I would expect a good chunk of them sit there un-reviewed based on our current rate of merging things, and that's fine. The developers as a whole have a total reviewing rate, which is unpredictable and finite by the very nature of this being an open-source project where no-one is obliged to do anything. If we want to adjust reviewing rules to increase 'productivity' that seems fine to me, but I don't think we should be doing it with the end goal of being able to review at the rate that code is submitted (or any particular other rate).</div></div></blockquote><div><br></div><div>The end goal is not to set a specific rate, but it is indeed to try to raise that rate up.  Obviously we could also throw our hands up and say nothing can be done about it, but that seems a bit defeatist.</div><div><br></div><div>Right now, I would bet that a lot of PRs go in with exactly two reviewers having gone through them and no other reviewer having even skimmed them.  If anything, I believe that the system I propose will increase the number of core-devs that'll have at least skimmed through the PR (all you need to do, if you want to participate, is to check your github notifications once every other week, whereas right now you'd actually need to pay attention to each PR as it is being submitted), and it is quite likely that 1 in-depth reviewer + 3-4 skim-throughs works as well, if not better, than 2 "in-depth" reviews (especially when the second one is sometimes "oh, that looks fine and it's already been approved once, let's just commit it").</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>re. large diff PRs, I find 100+ line PRs that are just copy and paste really hard to review - it's pretty dull for me trying to work out if code has been copy/pasted exactly without any errors (if there's an easier way to review stuff like this instead of checking every line please let me know!).</div></div></blockquote><div><br></div><div>Frankly, for a PR like that, I would just check that no function is missing and that everything is test-covered (without changes to the tests), and (in case you want to be sure) skim through the code to see that the PR author has not added a sneaky vulnerability that allows them to take over the user's computer... (In other words, I don't think a line-by-line check matters for such cases.)</div><div><br></div><div>Antony</div></div></div>
</blockquote></div>
</blockquote></div></div></div>