<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class=""><br class=""></div><div class="">There is a mathematical necessity here; if you have the commit bit and submit 50 PRs a month, you need to review 100 PRs a month, and merge or close 50 of those PRs a month.  If all the folks with commit bit do this, then obviously PRs will pile up.</div><div class=""><br class=""></div><div class="">Cheers,   Jody</div><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Jan 16, 2019, at  6:32 AM, Thomas Caswell <<a href="mailto:tcaswell@gmail.com" class="">tcaswell@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" style="caret-color: rgb(0, 0, 0); font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><div dir="ltr" class=""><div dir="ltr" class="">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").</div><div dir="ltr" class=""><br class=""></div><div class="">It seems to me that there are two classes of PRs that are getting grouped together here: small PRs that just fall through the cracks and big PRs that are hard to review, let's stay focused on the small PRs.</div><div class=""><br class=""></div><div class="">One habit related thing that may help is if we all bookmark:</div><div class=""><div class=""><br class=""></div><div class=""><a href="https://github.com/matplotlib/matplotlib/pulls?q=is%3Apr+is%3Aopen+review%3Aapproved+sort%3Aupdated-desc" class="">https://github.com/matplotlib/matplotlib/pulls?q=is%3Apr+is%3Aopen+review%3Aapproved+sort%3Aupdated-desc</a></div></div><div dir="ltr" class=""><br class=""></div><div class="">as where to start when we sit down to do some Matplotlib work.  I would also remind everyone that if you have a commit bit, then we trust you to use it!</div><div class=""><br class=""></div><div class="">As we write down the list of things that need to be true for single-review merging, it seems the effort to determine that a given PR checks (or does not) all of the boxes is equal to the effort to just review the PR.  This make me think that the biggest benefit of this change would be to get more eyes on PRs than any policy change.</div><div dir="ltr" class=""><br class=""></div><div dir="ltr" class="">To a fair degree this comes down to a project management / resource allocation problem (which are hard even in situations where everyone is paid!) <br class=""><div class=""><br class=""></div><div class="">Tom</div></div></div></div><br style="caret-color: rgb(0, 0, 0); font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><div class="gmail_quote" style="caret-color: rgb(0, 0, 0); font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;"><div dir="ltr" class="">On Wed, Jan 16, 2019 at 6:11 AM Antony Lee <<a href="mailto:antony.lee@institutoptique.fr" class="">antony.lee@institutoptique.fr</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div class="">Replying to Nelle's points first:</div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">It seems to me that this "uncontroversial but uninteresting" rule is currently not based on objective criteria. I'm personally not in favor of removing two core contributors approval for anything that modifies the API.  We have had too many close calls on PR almost merged that had backward incompatible changes: I realize that some core developers believe that we are not moving fast enough, on a project with hundreds of thousands of users, changes to the API should not be considered lightly. <br class=""></blockquote><div class=""><br class=""></div><div class="">To be clear (and even though it is probably common knowledge that I would like to evolve the API faster), there is no "secret agenda" of using this to sneak API breaks through the system.</div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">Here's an attempt at better defining an "uncontroversial but uninteresting" rule based on objective criteria:<br class="">- Has not been commented on by more than one core developer;<br class=""></blockquote><div class=""><br class=""></div><div class="">Sounds reasonable (with a reasonably flexible interpretation -- similarly to the "no activity since two weeks, excluding trivial chat"), basically "no other core dev expressed interest in reviewing it".</div><div class=""> <br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">- Doesn't change the public API;<br class="">- Doesn't add or remove a functionality;<br class="">- Doesn't add or remove a dependency;<br class=""></blockquote><div class=""><br class=""></div><div class="">I expect all changes of API to go through the normal route of deprecation and API change note, which even the first reviewer should make sure are present before sponsoring the PR for single-review-merge (as an aside, I guess the first reviewer could even say, "approving, but I don't want to take the responsibility for being the single reviewer"...).  So now you have a two week period where any other core dev passing by can just see whether there is an API change note and make their own judgment, including deciding to block the PR if necessary, based on that change note.</div><div class="">I guess it is reasonable to say "API deprecations or changes and new functionality should be clearly documented" to help that "skim-through" review.</div><div class="">As for *adding* functionality, I guess you would have rejected the ttc-font PR on that premise?  (Actually I now see that the additional functionality was actually not documented :))  Note that this is not some obscure edge case that no one cares about; it's a feature that has been requested for years on the tracker.</div><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">- Is fully tested;<br class=""></blockquote><div class=""> </div><div class="">Also proposed by Jody and now by David, sounds good to me.</div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">- All parameters are fully documented;<br class=""></blockquote><div class=""><br class=""></div><div class="">(sounds contradictory with "doesn't add a functionality"... if there is no new functionality it's unlikely there's a new parameter)</div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">- Doesn't fix a release-critical bug;<br class=""></blockquote><div class=""><br class=""></div><div class="">These tend to all be merged just before the next release is cut anyways (review dynamics are a bit different in these periods), so I wouldn't worry about it, but sure.</div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">- 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.<br class=""></blockquote><div class=""><br class=""></div><div class="">I don't actually think this is a good rule; the pdf/ps merge PR is +138 lines, -182 lines but as explained above the change is fairly trivial; obviously I could have split it into smaller chunks but if anything that would have made it harder to review.</div><div class="">Conversely (to use an example I have already raised elsewhere)<span class="Apple-converted-space"> </span><a href="https://github.com/matplotlib/matplotlib/pull/11530" target="_blank" class="">https://github.com/matplotlib/matplotlib/pull/11530</a><span class="Apple-converted-space"> </span>(making sure that LocationEvent.x and LocationEvent.y are integers (as they refer to pixel locations) rather than floats) is 2 lines of changes in the codebase and 20 lines of test, was reviewed by 2.5 core devs (I left a comment in passing) and we all missed that there was "problem" with it (it broke mplcursors and some other pieces of code I have that assume that it is possible to synthetize LocationEvents pointing to arbitrary places in the canvas).  (Not to derail this discussion into how to evaluate API breaks once again... but I think these kinds of changes are much more insidious than, say, removing a function, as the latter would just trigger an AttributeError that is easily noticed and at worst you just go back to the previous Matplotlib release, whereas the LocationEvent change was only caught because it was covered by the mplcursors test suite; otherwise I'd have been silently generating wrong results due to it...)</div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">To be honest, it also doesn't help that some people have 50+ pull requests open (Antony? :p).</blockquote><div class=""><br class=""></div><div class="">I guess if the way to fix Matplotlib is to not contribute to Matplotlib anymore, we have a bigger problem.  In fact, I have some pretty big patches fixing long-standing issues (e.g. for #8550 I have a pure-Python reimplementation of the fontconfig font weight deduction algorithm, translated from fontconfig's C implementation) that I don't even bother turning into PRs, as I know they have no chance of being reviewed and merged under the current system at least, because no one would care (see also the bus-factor point below).</div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">It also doesn't help that some PR contain the same code (13117, 13128) and are being hold off until the other one is merged (also, why is 13117 being hold off for a PR that has been opened afterwards?). IMO, if a PR can be merged, it should be merged.</blockquote><div class=""><br class=""></div><div class="">That's discussed in <a href="https://github.com/matplotlib/matplotlib/pull/13117#discussion_r245503693" target="_blank" class="">https://github.com/matplotlib/matplotlib/pull/13117#discussion_r245503693</a><span class="Apple-converted-space"> </span>(FTR I actually agreed on merging 13117 directly, but apparently that's an API break...)</div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">But we shouldn't make too many exceptions to the two core contributors rules. It's easier to have two core contributor review a pull request, than to try to fix API issues on code that has been merged and released. Having two pairs of eyes on a PR is also a good way to avoid having a bus factor of 1 on some areas of the library.</blockquote><div class=""><br class=""></div><div class="">There are already huge and central chunks of the library (Agg internals, PDF/PS) that have a bus factor close to zero.  If anything, I hope that this "drive-by review" system will encourage reviewers to start looking at places they are less familiar with ("oh yes, I guess this change makes sense" -- see Eric's reply above) and decrease that.</div><div class=""><br class=""></div><div class="">And then to David's (the code-coverage point is already covered above):</div><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">I think it is good remember that (as far as I know) everyone submitting PRs and everyone reviewing them isn't paid and does so in their spare time; therefore is no obligation for anyone to review anything on any particular timescale. One thing that hasn't been discussed is how to increase the reviewing capacity of the community, which I guess would involve working out how to attract and retain more regular contributors.</blockquote><div class=""><br class=""></div><div class="">Given the premises "no one is paid to review PRs" and "the reviewing capacity is too small" (which I agree with), I think there are two reasonable solutions:</div><div class="">- Make PRs easier to merge (this proposal),</div><div class="">- Increasing to number of core devs (your proposal, but it's not clear to me whether this number can increase significantly in a short period of time...).</div><div class=""><br class=""></div><div class="">Antony</div></div></div></div></div></div></div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="gmail-m_3030517262419707027gmail-m_-3441066552313079160gmail_attr">On Wed, Jan 16, 2019 at 10:34 AM David Stansby <<a href="mailto:dstansby@gmail.com" target="_blank" class="">dstansby@gmail.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div dir="auto" class=""><div class="">I think it is good remember that (as far as I know) everyone submitting PRs and everyone reviewing them isn't paid and does so in their spare time; therefore is no obligation for anyone to review anything on any particular timescale. One thing that hasn't been discussed is how to increase the reviewing capacity of the community, which I guess would involve working out how to attract and retain more regular contributors.</div><div dir="auto" class=""><br class=""></div><div dir="auto" class="">re. the proposed changes, I'm +1 modulo objective criteria being agreed upon. Personally I agree with Nelle's list, with the addition "has 100% code coverage".</div><div dir="auto" class=""><br class=""></div><div dir="auto" class="">David</div><div dir="auto" class=""><br class=""></div><div dir="auto" class=""><br class=""><br class=""><div class="gmail_quote" dir="auto"><div dir="ltr" class="">On Tue, 15 Jan 2019, 23:46 Nelle Varoquaux <<a href="mailto:nelle.varoquaux@gmail.com" target="_blank" class="">nelle.varoquaux@gmail.com</a><span class="Apple-converted-space"> </span>wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div class="">For the way GitHub sorts pull requests, it is indeed a problem, but there's a way to sort by "most recently updated." I sometimes use it to make sure that I review recently updated pull request that don't appear on the first page.</div><div class=""><br class=""></div></div><div class="">It seems to me that this "uncontroversial but uninteresting" rule is currently not based on objective criteria. I'm personally not in favor of removing two core contributors approval for anything that modifies the API.  We have had too many close calls on PR almost merged that had backward incompatible changes: I realize that some core developers believe that we are not moving fast enough, on a project with hundreds of thousands of users, changes to the API should not be considered lightly.<span class="Apple-converted-space"> </span><br class=""></div><div class=""><br class=""></div><div class="">Here's an attempt at better defining an "uncontroversial but uninteresting" rule based on objective criteria:</div><div class="">- Has not been commented on by more than one core developer;<br class=""></div><div class="">- Doesn't change the<span class="Apple-converted-space"> </span><b class="">public</b><span class="Apple-converted-space"> </span>API;</div><div class="">- Doesn't add or remove a functionality;</div><div class="">- Doesn't add or remove a dependency;<br class=""></div><div class="">- Is fully tested;</div><div class="">- All parameters are fully documented;</div><div class="">- Doesn't fix a release-critical bug;<br class=""></div><div class="">- 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.<br class=""></div><div dir="ltr" class=""><br class=""><div class=""><div dir="ltr" class="">To be honest, it also doesn't help that some people have 50+ pull requests open (Antony? :p). It also doesn't help that some PR contain the same code (13117, 13128) and are being hold off until the other one is merged (also, why is 13117 being hold off for a PR that has been opened afterwards?). IMO, if a PR can be merged, it should be merged. But we shouldn't make too many exceptions to the two core contributors rules. It's easier to have two core contributor review a pull request, than to try to fix API issues on code that has been merged and released. Having two pairs of eyes on a PR is also a good way to avoid having a bus factor of 1 on some areas of the library.<br class=""></div><div class=""><br class=""></div></div></div></div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Tue, 15 Jan 2019 at 15:19, Thomas Caswell <<a href="mailto:tcaswell@gmail.com" rel="noreferrer" target="_blank" class="">tcaswell@gmail.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr" class=""><div dir="ltr" class="">I think a distinction between "no one cares" and "no one has bandwidth" needs to be made.<div class=""><br class=""></div><div class="">If you look at the 'pulse' page, we merging 100+ PRs a month (<a href="https://github.com/matplotlib/matplotlib/pulse/monthly" rel="noreferrer" target="_blank" class="">https://github.com/matplotlib/matplotlib/pulse/monthly</a>) so reviews are happening, the issue is just that some are falling through the cracks.</div><div class=""><br class=""></div><div class="">Tom</div></div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Tue, Jan 15, 2019 at 4:45 PM Antony Lee <<a href="mailto:antony.lee@institutoptique.fr" rel="noreferrer" target="_blank" class="">antony.lee@institutoptique.fr</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr" class=""><div dir="ltr" class="">Eric,<div class="">Neither the ttc font, nor the pdf/ps common code PRs actually touch any "complex" point regarding fonts or pdf/ps.  I have described the ttc font in my previous message; the pdf/ps PR is really just, look, these two classes share 80 lines worth of code that's literally copy-pasted, let's just put that in a common parent class and inherit from it.</div><div class="">Conversely, one font-related PR for which I actually have low hopes of getting properly reviewed (except on an "sure, I trust you" basis) is <a href="https://github.com/matplotlib/matplotlib/pull/12928" rel="noreferrer" target="_blank" class="">https://github.com/matplotlib/matplotlib/pull/12928</a>, which actually touches a rather complex point about font encodings (some of which I had to clarify on the FreeType mailing list).  (If anyone wants to have a look at it, though... :p)</div><div class="">Antony</div></div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="gmail-m_3030517262419707027gmail-m_-3441066552313079160gmail-m_2738842794805438613m_3901295563710045304gmail-m_8784059746141603411gmail-m_-150229076235639764gmail_attr">On Tue, Jan 15, 2019 at 10:36 PM Eric Firing <<a href="mailto:efiring@hawaii.edu" rel="noreferrer" target="_blank" class="">efiring@hawaii.edu</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">Antony,<br class=""><br class="">Your examples illustrate that the problem is often that we don't have<span class="Apple-converted-space"> </span><br class="">enough people who understand some areas, like fonts and pdf/ps file<span class="Apple-converted-space"> </span><br class="">formats, to get 2 thorough and knowledgeable reviews in a reasonable time.<br class=""><br class="">I'm fine with your proposed rule.  It should help at least a little bit.<br class=""><br class="">Eric<br class=""><br class=""><br class="">On 2019/01/15 9:11 AM, Antony Lee wrote:<br class="">> I'm obviously more aware of my own PRs, so here are a few I'd have put<span class="Apple-converted-space"> </span><br class="">> up for single-review-merge (note that it's quite possible that they'd<span class="Apple-converted-space"> </span><br class="">> have attracted more attention and gotten merged faster under that<span class="Apple-converted-space"> </span><br class="">> system; that's also the goal...).  Obviously other devs may think about<span class="Apple-converted-space"> </span><br class="">> other PRs that could have benefitted from the same process.<br class="">><span class="Apple-converted-space"> </span><br class="">> -<span class="Apple-converted-space"> </span><a href="https://github.com/matplotlib/matplotlib/pull/9787" rel="noreferrer noreferrer" target="_blank" class="">https://github.com/matplotlib/matplotlib/pull/9787</a><span class="Apple-converted-space"> </span>adds support for<span class="Apple-converted-space"> </span><br class="">> the ttc font format (requested since 2014) for png/svg output (that's<span class="Apple-converted-space"> </span><br class="">> provided basically for free by FreeType) but not for pdf/ps (that would<span class="Apple-converted-space"> </span><br class="">> require more difficult changes).  The entire PR comes down to:<br class="">>    * adding "ttc" as an extension that should be treated like "ttf"/"otf",<br class="">>    * adding error handling to pdf/ps to signal these formats are not<span class="Apple-converted-space"> </span><br class="">> supported there,<br class="">>    * tests.<br class="">>    The PR sat open for 10 months before a first positive review (because<span class="Apple-converted-space"> </span><br class="">> no one cares about fonts, I guess) and took another 3 months to attract<span class="Apple-converted-space"> </span><br class="">> a second positive review.<br class="">><span class="Apple-converted-space"> </span><br class="">> -<span class="Apple-converted-space"> </span><a href="https://github.com/matplotlib/matplotlib/pull/12472" rel="noreferrer noreferrer" target="_blank" class="">https://github.com/matplotlib/matplotlib/pull/12472</a><span class="Apple-converted-space"> </span>does a fairly<span class="Apple-converted-space"> </span><br class="">> trivial fix to fontList.json to make it reusable for Matplotlibs<span class="Apple-converted-space"> </span><br class="">> installed in different venvs, got a positive review in one day and<span class="Apple-converted-space"> </span><br class="">> waited another 2.5 months for a second positive review.<br class="">><span class="Apple-converted-space"> </span><br class="">> -<span class="Apple-converted-space"> </span><a href="https://github.com/matplotlib/matplotlib/pull/9867" rel="noreferrer noreferrer" target="_blank" class="">https://github.com/matplotlib/matplotlib/pull/9867</a><span class="Apple-converted-space"> </span>deduplicates<span class="Apple-converted-space"> </span><br class="">> completely duplicated code between the pdf and ps backends, mostly for<span class="Apple-converted-space"> </span><br class="">> maintainability's sake; it took 7.5 months (and 5 rebases due to<span class="Apple-converted-space"> </span><br class="">> conflicts) to get a first positive review and another 6 (and around as<span class="Apple-converted-space"> </span><br class="">> many rebases) to get a second one.<br class="">><span class="Apple-converted-space"> </span><br class="">> -----<br class="">><span class="Apple-converted-space"> </span><br class="">> As for the specifics (as I see it):<br class="">> - Someone opens a PR, it gets a positive review, no negative review, and<span class="Apple-converted-space"> </span><br class="">> no activity occurs for two weeks (significant activity -- excluding<span class="Apple-converted-space"> </span><br class="">> trivial chat on the thread).<br class="">> - Sponsor (first reviewer, or author if a committer) *pings* all devs by<span class="Apple-converted-space"> </span><br class="">> mentioning @matplotlib/developers on the PR thread with the intent to<span class="Apple-converted-space"> </span><br class="">> single-review-merge, and labels the PR accordingly.<br class="">> - The PR can still be reviewed/merged/rejected by the normal review<span class="Apple-converted-space"> </span><br class="">> process.  However, if no one explicitly opposes the merge within two<span class="Apple-converted-space"> </span><br class="">> weeks, anyone can merge it at that point (including by self-merge).<br class="">><span class="Apple-converted-space"> </span><br class="">> I would not exclude all API changes from the process.  For example,<span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span><a href="https://github.com/matplotlib/matplotlib/pull/13173/commits/e90d264f3e5a263ef57243a2af86b46ab74ccc16" rel="noreferrer noreferrer" target="_blank" class="">https://github.com/matplotlib/matplotlib/pull/13173/commits/e90d264f3e5a263ef57243a2af86b46ab74ccc16</a><br class="">> deprecates (and prepares for deletion) two parameters to imshow() that<span class="Apple-converted-space"> </span><br class="">> have had no effect since 2006.  Let's pretend for a second this commit<span class="Apple-converted-space"> </span><br class="">> was a single independent PR (and not an example to showcase the<span class="Apple-converted-space"> </span><br class="">> parameter-deprecating decorator...); if it started being forgotten I<span class="Apple-converted-space"> </span><br class="">> would have proposed it for single-review-merge (again, note that if<span class="Apple-converted-space"> </span><br class="">> you're worried about the change, you don't need to reject the PR; you<span class="Apple-converted-space"> </span><br class="">> can just say, I think this API change needs to be approved by a second<span class="Apple-converted-space"> </span><br class="">> reviewer and that'll block the single-review-merge just as well).<br class="">><span class="Apple-converted-space"> </span><br class="">> Antony<br class="">><span class="Apple-converted-space"> </span><br class="">> On Tue, Jan 15, 2019 at 7:46 PM Nelle Varoquaux<span class="Apple-converted-space"> </span><br class="">> <<a href="mailto:nelle.varoquaux@gmail.com" rel="noreferrer" target="_blank" class="">nelle.varoquaux@gmail.com</a><span class="Apple-converted-space"> </span><mailto:<a href="mailto:nelle.varoquaux@gmail.com" rel="noreferrer" target="_blank" class="">nelle.varoquaux@gmail.com</a>>> wrote:<br class="">><span class="Apple-converted-space"> </span><br class="">>     The proposed scheme by Paul doesn't seem reasonable to me. Core<br class="">>     contributor A or committers needs to actively reach out to other<br class="">>     core contributors: labeling is not enough IMO.<br class="">><span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span><br class="">>     On Tue, 15 Jan 2019 at 10:37, Jody Klymak <<a href="mailto:jklymak@uvic.ca" rel="noreferrer" target="_blank" class="">jklymak@uvic.ca</a><br class="">>     <mailto:<a href="mailto:jklymak@uvic.ca" rel="noreferrer" target="_blank" class="">jklymak@uvic.ca</a>>> wrote:<br class="">><span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span><br class="">>>         On 15 Jan 2019, at 10:20, Paul Hobson <<a href="mailto:pmhobson@gmail.com" rel="noreferrer" target="_blank" class="">pmhobson@gmail.com</a><br class="">>>         <mailto:<a href="mailto:pmhobson@gmail.com" rel="noreferrer" target="_blank" class="">pmhobson@gmail.com</a>>> wrote:<br class="">>><br class="">>>         I think this would be a good policy as well. Can I get some<br class="">>>         clarification on the flow? The way I understand it:<br class="">>><br class="">>>         1) Someone (committer, contributor, or first-timer) opens a<br class="">>>         simple PR<br class="">>>         2) Committer A reviews it, adds a "single-review-merge" label,<br class="">>>         then digitally walks away<br class="">>>         3) No less than two weeks later, Committer B sees the PR,<br class="">>>         notices the label, opens it up, and merges without reviewing.<br class="">><span class="Apple-converted-space"> </span><br class="">>         If there is a Committer B, I think they should at least look at<br class="">>         the PR to make sure it doesn’t have any hidden API changes, or<br class="">>         introduce anything hostile.<br class="">><span class="Apple-converted-space"> </span><br class="">>         The point here is that getting the third person to look at a PR<br class="">>         is proving quite difficult, and if the rest of the folks with<br class="">>         the commit bit haven’t looked at a PR for a month, even after<br class="">>         being warned, then silence indicates consent.<br class="">><span class="Apple-converted-space"> </span><br class="">>         Note that only folks with the commit bit can label PRs, so<br class="">>         anyone flagging a PR like this is implicitly trusted to not be<br class="">>         wreaking havoc, and not doing this for major changes.  Since we<br class="">>         all curate our own PRs, I rather expect that the PR contributor<br class="">>         will often be the person who adds the flag.  Whether we want to<br class="">>         allow a self-merge at that point is up for debate, but if not,<br class="">>         then folks need to get in the habit of looking at old flagged<br class="">>         PRs and merging them.<br class="">><span class="Apple-converted-space"> </span><br class="">>         I think a huge problem we have is the LIFO default sort of the<br class="">>         github PR queue, and that any PR not on the first page might as<br class="">>         well be closed for all the attention it will get without<br class="">>         constant nagging.<br class="">><span class="Apple-converted-space"> </span><br class="">>         Cheers,   Jody<br class="">><span class="Apple-converted-space"> </span><br class="">>>         Does that capture a successful "single-review-merge" lifecycle?<br class="">>>         -paul<br class="">>><br class="">>>         On Tue, Jan 15, 2019 at 9:57 AM Nelle Varoquaux<br class="">>>         <<a href="mailto:nelle.varoquaux@gmail.com" rel="noreferrer" target="_blank" class="">nelle.varoquaux@gmail.com</a><span class="Apple-converted-space"> </span><mailto:<a href="mailto:nelle.varoquaux@gmail.com" rel="noreferrer" target="_blank" class="">nelle.varoquaux@gmail.com</a>>><br class="">>>         wrote:<br class="">>><br class="">>>             Hello,<br class="">>><br class="">>>             Overall, I think this is a good idea, but I would like<br class="">>>             specifics on what "uncontroversial but uninteresting" PR<br class="">>>             mean. IMO, that should exclude any PR with API changes.<br class="">>><br class="">>>             Cheers,<br class="">>>             N<br class="">>><br class="">>>             On Tue, 15 Jan 2019 at 06:00, Antony Lee<br class="">>>             <<a href="mailto:anntzer.lee@gmail.com" rel="noreferrer" target="_blank" class="">anntzer.lee@gmail.com</a><span class="Apple-converted-space"> </span><mailto:<a href="mailto:anntzer.lee@gmail.com" rel="noreferrer" target="_blank" class="">anntzer.lee@gmail.com</a>>> wrote:<br class="">>><br class="">>>                 Hi all,<br class="">>><br class="">>>                 During the weekly dev call, I proposed to amend the PR<br class="">>>                 merge rules, following an initial comment on Github<br class="">>>                 [<a href="https://github.com/matplotlib/matplotlib/pull/13173#issuecomment-453921220" rel="noreferrer noreferrer" target="_blank" class="">https://github.com/matplotlib/matplotlib/pull/13173#issuecomment-453921220</a>].<span class="Apple-converted-space"> </span><br class="">>>                 There was general agreement among the devs present<br class="">>>                 (Tom, Hannah, Jody, and myself), so I'm putting it<br class="">>>                 here for discussion.  The objective of this change is<br class="">>>                 to prevent "uncontroversial but uninteresting" PRs<br class="">>>                 from falling into oblivion, and try to decrease the<br class="">>>                 size of the open PR stack.<br class="">>><br class="">>>                 The current rule is that a PR needs positive reviews<br class="">>>                 (from two committers, and excluding the author if a<br class="">>>                 committer) to be merged, except that doc-only PRs<br class="">>>                 (docstrings, rst) only needs a single positive review.<br class="">>><br class="">>>                 I am proposing that, if a (non-doc) PR already has one<br class="">>>                 positive review, but no activity on that PR has<br class="">>>                 occurred for two weeks (exact time interval up to<br class="">>>                 bikeshedding) [Jody suggests: and the PR has 100% code<br class="">>>                 coverage], then a committer (either the first<br class="">>>                 reviewer, or the author if a committer) can suggest<br class="">>>                 that it be merged on the basis of that single review.<span class="Apple-converted-space"> </span><br class="">>>                 To do so, the "sponsor" should ping all developers<br class="">>>                 (@matplotlib/developers) on that issue indicating that<br class="">>>                 intent, and add a "single-review-merge" label on the<br class="">>>                 PR (so that these PRs can easily be found).<span class="Apple-converted-space"> </span><br class="">>>                 Committers are encouraged to review the PR to accept<br class="">>>                 and merge it or reject it or request changes on it;<br class="">>>                 but they can also just indicate that they consider the<br class="">>>                 PR sufficiently complex that a proper second review is<br class="">>>                 needed before merging, or request an extension, etc.<span class="Apple-converted-space"> </span><br class="">>>                 To do so they should still leave a "reject" review,<br class="">>>                 even if just saying "objecting to single-review<br class="">>>                 merged; anyone can dismiss after a second review".<span class="Apple-converted-space"> </span><br class="">>>                 However, if within another two weeks, no committer<br class="">>>                 voiced any objection (explicitly, i.e. by rejecting),<br class="">>>                 then the PR can indeed be merged on the basis of that<br class="">>>                 single review.<br class="">>><br class="">>>                 To avoid overwhelming the system, any committer can<br class="">>>                 only "sponsor" a single PR at a time.<br class="">>><br class="">>>                 Thoughts?<br class="">>><br class="">>>                 Antony<br class="">>>                 _______________________________________________<br class="">>>                 Matplotlib-devel mailing list<br class="">>>                 <a href="mailto:Matplotlib-devel@python.org" rel="noreferrer" target="_blank" class="">Matplotlib-devel@python.org</a><br class="">>>                 <mailto:<a href="mailto:Matplotlib-devel@python.org" rel="noreferrer" target="_blank" class="">Matplotlib-devel@python.org</a>><br class="">>>                 <a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" rel="noreferrer noreferrer" target="_blank" class="">https://mail.python.org/mailman/listinfo/matplotlib-devel</a><br class="">>><br class="">>>             _______________________________________________<br class="">>>             Matplotlib-devel mailing list<br class="">>>             <a href="mailto:Matplotlib-devel@python.org" rel="noreferrer" target="_blank" class="">Matplotlib-devel@python.org</a><br class="">>>             <mailto:<a href="mailto:Matplotlib-devel@python.org" rel="noreferrer" target="_blank" class="">Matplotlib-devel@python.org</a>><br class="">>>             <a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" rel="noreferrer noreferrer" target="_blank" class="">https://mail.python.org/mailman/listinfo/matplotlib-devel</a><br class="">>><br class="">>>         _______________________________________________<br class="">>>         Matplotlib-devel mailing list<br class="">>>         <a href="mailto:Matplotlib-devel@python.org" rel="noreferrer" target="_blank" class="">Matplotlib-devel@python.org</a><span class="Apple-converted-space"> </span><mailto:<a href="mailto:Matplotlib-devel@python.org" rel="noreferrer" target="_blank" class="">Matplotlib-devel@python.org</a>><br class="">>>         <a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" rel="noreferrer noreferrer" target="_blank" class="">https://mail.python.org/mailman/listinfo/matplotlib-devel</a><br class="">><span class="Apple-converted-space"> </span><br class="">>         --<br class="">>         Jody Klymak<br class="">>         <a href="http://web.uvic.ca/~jklymak/" rel="noreferrer noreferrer" target="_blank" class="">http://web.uvic.ca/~jklymak/</a><br class="">><span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span><br class="">>     _______________________________________________<br class="">>     Matplotlib-devel mailing list<br class="">>     <a href="mailto:Matplotlib-devel@python.org" rel="noreferrer" target="_blank" class="">Matplotlib-devel@python.org</a><span class="Apple-converted-space"> </span><mailto:<a href="mailto:Matplotlib-devel@python.org" rel="noreferrer" target="_blank" class="">Matplotlib-devel@python.org</a>><br class="">>     <a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" rel="noreferrer noreferrer" target="_blank" class="">https://mail.python.org/mailman/listinfo/matplotlib-devel</a><br class="">><span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span><br class="">> _______________________________________________<br class="">> Matplotlib-devel mailing list<br class="">><span class="Apple-converted-space"> </span><a href="mailto:Matplotlib-devel@python.org" rel="noreferrer" target="_blank" class="">Matplotlib-devel@python.org</a><br class="">><span class="Apple-converted-space"> </span><a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" rel="noreferrer noreferrer" target="_blank" class="">https://mail.python.org/mailman/listinfo/matplotlib-devel</a><br class="">><span class="Apple-converted-space"> </span><br class=""><br class="">_______________________________________________<br class="">Matplotlib-devel mailing list<br class=""><a href="mailto:Matplotlib-devel@python.org" rel="noreferrer" target="_blank" class="">Matplotlib-devel@python.org</a><br class=""><a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" rel="noreferrer noreferrer" target="_blank" class="">https://mail.python.org/mailman/listinfo/matplotlib-devel</a><br class=""></blockquote></div>_______________________________________________<br class="">Matplotlib-devel mailing list<br class=""><a href="mailto:Matplotlib-devel@python.org" rel="noreferrer" target="_blank" class="">Matplotlib-devel@python.org</a><br class=""><a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" rel="noreferrer noreferrer" target="_blank" class="">https://mail.python.org/mailman/listinfo/matplotlib-devel</a><br class=""></blockquote></div><br clear="all" class=""><div class=""><br class=""></div>--<span class="Apple-converted-space"> </span><br class=""><div dir="ltr" class="gmail-m_3030517262419707027gmail-m_-3441066552313079160gmail-m_2738842794805438613m_3901295563710045304gmail-m_8784059746141603411gmail_signature">Thomas Caswell<br class=""><a href="mailto:tcaswell@gmail.com" rel="noreferrer" target="_blank" class="">tcaswell@gmail.com</a></div>_______________________________________________<br class="">Matplotlib-devel mailing list<br class=""><a href="mailto:Matplotlib-devel@python.org" rel="noreferrer" target="_blank" class="">Matplotlib-devel@python.org</a><br class=""><a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" rel="noreferrer noreferrer" target="_blank" class="">https://mail.python.org/mailman/listinfo/matplotlib-devel</a><br class=""></blockquote></div>_______________________________________________<br class="">Matplotlib-devel mailing list<br class=""><a href="mailto:Matplotlib-devel@python.org" rel="noreferrer" target="_blank" class="">Matplotlib-devel@python.org</a><br class=""><a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" rel="noreferrer noreferrer" target="_blank" class="">https://mail.python.org/mailman/listinfo/matplotlib-devel</a><br class=""></blockquote></div></div></div></blockquote></div></blockquote></div><br clear="all" style="caret-color: rgb(0, 0, 0); font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><div style="caret-color: rgb(0, 0, 0); font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class=""></div><span style="caret-color: rgb(0, 0, 0); font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">--<span class="Apple-converted-space"> </span></span><br style="caret-color: rgb(0, 0, 0); font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><div dir="ltr" class="gmail_signature" style="caret-color: rgb(0, 0, 0); font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;">Thomas Caswell<br class=""><a href="mailto:tcaswell@gmail.com" target="_blank" class="">tcaswell@gmail.com</a></div><span style="caret-color: rgb(0, 0, 0); font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">_______________________________________________</span><br style="caret-color: rgb(0, 0, 0); font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Matplotlib-devel mailing list</span><br style="caret-color: rgb(0, 0, 0); font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><a href="mailto:Matplotlib-devel@python.org" style="font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">Matplotlib-devel@python.org</a><br style="caret-color: rgb(0, 0, 0); font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" style="font-family: LucidaSans-Typewriter; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">https://mail.python.org/mailman/listinfo/matplotlib-devel</a></div></blockquote></div><br class=""></body></html>