<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><br></div><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><br></div><div>David<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 16 Jan 2019 at 17:54, Antony Lee <<a href="mailto:antony.lee@institutoptique.fr">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"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail-m_-5631788052206915634gmail_attr">On Wed, Jan 16, 2019 at 6:26 PM Nelle Varoquaux <<a href="mailto:nelle.varoquaux@gmail.com" target="_blank">nelle.varoquaux@gmail.com</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"><div><br></div><div>I didn't mean to imply that you had a secret agenda in changing the API in a backward incompatible way. But mistakes happen, and looking for 
backward incompatibility has not been a top priority for some core 
developers recently, and  over time it creates a culture where looking 
carefully for backward incompatibility is not part of our reviewing 
process. Having two core developers review changes limits the chance of 
such pull requests being merged with backward incompatibility. I also 
think that every API change, including the addition of keywords, needs 
to be considered carefully. Part of the issues we are having in terms of
 maintenance are that too many functions are public and many of those 
don't have a consistent API with the rest of the library.</div></div></div></blockquote><div><br></div><div>Again, you may note that I have fairly few PRs that introduce new functionality; most of the API changes I PR'd actually deprecate and later remove functions that "should not have been public" (I have sort of given up on making things consistent in an backcompatible manner).  So in that respect I am mostly trying to improve maintanability...</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 dir="ltr"><div>To summarize the points that have been agreed upon and the points that still need to be discussed. I've removed the one about documentation, but if we accept API changes in this system, it should probably added back as a point to be discussed.<br></div><div><br></div><div>Agreed upon:<br></div><div>- Has not been commented on by more than one core developer;</div><div>- Doesn't fix a release-critical bug;</div><div>- Is fully tested.<br></div><div><br></div><div>Still needs to be discussed:<br></div><div>1. Doesn't change the public API;<br>2. Doesn't add or remove a functionality;<br>3. Doesn't add or remove a dependency;</div><div>4. Is less than XX lines of code. (Long PR are harder to review, and thus
 the likelihood of one core dev not noticing a problem is higher). 
Threshold needs to be defined.</div><div><br></div><div>I'd also like to add:</div><div>5. All CI passes (because we have, in the past, ignored some errors on Travis when we felt tests were flaky)</div></div></div></blockquote><div><br></div><div>Agreed on 5.</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 dir="ltr"><div>I maintain that any pull requests that add or remove a functionality should be reviewed by 2 core developers. I guess our BDFL may have to make the final call on that one :)</div></div></div></blockquote><div><br></div><div>Happy to let Tom decide, but again I would say that "add or remove a functionality" is not always an objective feature either (e.g., does the LocationEvent-integer PR qualify?).</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>My argument for 4. is that long pull requests are harder to review, even if they are trivial (there are studies on this.). It is thus easier to make a mistake in reviewing.</div></div></blockquote><div><br></div><div>As I already mentioned, there have been cases of 2-line PRs (+ 20 lines of *new* tests) making backwards incompatible changes, whereas some 100+ lines PR are essentially copy-paste trivialities, so I really don't buy the argument.</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>In terms of number of pull request per core dev, it is just that more pull requests dilute our effort in reviewing. The more someone has pull request open, the less likely they are to be reviewed and the more likely that developer is to "forget" about them. I don't think we should have a rule on the number of pull request open per person, but on the different project I contribute to, I have a rule of thumb to not have more than 5 pull requests opened. It forces me to finish up the work on the pull requests I have opened before working on something else.</div></div></blockquote><div><br></div><div>Frankly it's not clear what "finish up the work" is supposed to mean.  If you look at the PRs I've mentioned above they have been merged with (close to) no changes (except for repeated rebases on master...) after the 6-to-12-month review period, so essentially all the work was done from the beginning.</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>As for David's point, it just happens that some collaborators and I are working on understanding factors for newcomer retention and developer burnout. We've got preliminary results that we are not ready to share but we are hoping that it'll help on that point.</div></div></blockquote><div><br></div><div>Happy to hear anything that could be done to help on that point.  But I guess I'll take advantage of this to reply to Tom's remark as well</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">[Tom] I want to push back strongly on the notion that "did not review --> does not care".  As as already been discussed above there are many other reasons someone does not comment (for example "I _do_ care about this, but I {do not have time, am too tired and cranky, ...} to properly review this now").</blockquote><div> </div><div>I'm not pointing fingers at anyone not doing their review share, and once again totally agree with David's point regarding no one being paid.</div><div>However, you have to realize that the consequences of insufficient review volume is that real improvements are not getting in (either because the PRs stall out, or (in my case) because I don't even bother PR'ing them anymore and just live-patch matplotlib locally), and saying "it's no-one's fault" (which is true) is not going to help.</div><div><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>Cheers,</div><div>N</div></div></blockquote><div><br></div><div>Antony</div></div></div></div></div>
</blockquote></div>