<div dir="ltr"><div dir="ltr"><div>I'm obviously more aware of my own PRs, so here are a few I'd have put up for single-review-merge (note that it's quite possible that they'd have attracted more attention and gotten merged faster under that system; that's also the goal...).  Obviously other devs may think about other PRs that could have benefitted from the same process.</div><div><br></div><div>- <a href="https://github.com/matplotlib/matplotlib/pull/9787">https://github.com/matplotlib/matplotlib/pull/9787</a> adds support for the ttc font format (requested since 2014) for png/svg output (that's provided basically for free by FreeType) but not for pdf/ps (that would require more difficult changes).  The entire PR comes down to:</div><div>  * adding "ttc" as an extension that should be treated like "ttf"/"otf",</div><div>  * adding error handling to pdf/ps to signal these formats are not supported there,</div><div>  * tests.</div><div>  The PR sat open for 10 months before a first positive review (because no one cares about fonts, I guess) and took another 3 months to attract a second positive review.</div><div><br></div><div>- <a href="https://github.com/matplotlib/matplotlib/pull/12472">https://github.com/matplotlib/matplotlib/pull/12472</a> does a fairly trivial fix to fontList.json to make it reusable for Matplotlibs installed in different venvs, got a positive review in one day and waited another 2.5 months for a second positive review.</div><div><br></div><div>- <a href="https://github.com/matplotlib/matplotlib/pull/9867">https://github.com/matplotlib/matplotlib/pull/9867</a> deduplicates completely duplicated code between the pdf and ps backends, mostly for maintainability's sake; it took 7.5 months (and 5 rebases due to conflicts) to get a first positive review and another 6 (and around as many rebases) to get a second one.</div><div><br></div><div>-----</div><div><br></div><div>As for the specifics (as I see it):</div><div>- Someone opens a PR, it gets a positive review, no negative review, and no activity occurs for two weeks (significant activity -- excluding trivial chat on the thread).</div><div>- Sponsor (first reviewer, or author if a committer) *pings* all devs by mentioning @matplotlib/developers on the PR thread with the intent to single-review-merge, and labels the PR accordingly.</div><div>- The PR can still be reviewed/merged/rejected by the normal review process.  However, if no one explicitly opposes the merge within two weeks, anyone can merge it at that point (including by self-merge).</div><div><br></div><div>I would not exclude all API changes from the process.  For example, <a href="https://github.com/matplotlib/matplotlib/pull/13173/commits/e90d264f3e5a263ef57243a2af86b46ab74ccc16">https://github.com/matplotlib/matplotlib/pull/13173/commits/e90d264f3e5a263ef57243a2af86b46ab74ccc16</a> deprecates (and prepares for deletion) two parameters to imshow() that have had no effect since 2006.  Let's pretend for a second this commit was a single independent PR (and not an example to showcase the parameter-deprecating decorator...); if it started being forgotten I would have proposed it for single-review-merge (again, note that if you're worried about the change, you don't need to reject the PR; you can just say, I think this API change needs to be approved by a second reviewer and that'll block the single-review-merge just as well).</div><div><br></div><div>Antony</div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jan 15, 2019 at 7:46 PM Nelle Varoquaux <<a href="mailto:nelle.varoquaux@gmail.com">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>The proposed scheme by Paul doesn't seem reasonable to me. Core contributor A or committers needs to actively reach out to other core contributors: labeling is not enough IMO.<br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, 15 Jan 2019 at 10:37, Jody Klymak <<a href="mailto:jklymak@uvic.ca" target="_blank">jklymak@uvic.ca</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><br><div><br><blockquote type="cite"><div>On 15 Jan 2019, at 10:20, Paul Hobson <<a href="mailto:pmhobson@gmail.com" target="_blank">pmhobson@gmail.com</a>> wrote:</div><br class="gmail-m_-6427996525205526815gmail-m_2085301446449759380gmail-m_-5445635510087654823gmail-m_-867484032130323208Apple-interchange-newline"><div><div dir="ltr"><div>I think this would be a good policy as well. Can I get some clarification on the flow? The way I understand it:</div><div><br></div><div>1) Someone (committer, contributor, or first-timer) opens a simple PR</div><div>2) Committer A reviews it, adds a "single-review-merge" label, then digitally walks away</div><div>3) No less than two weeks later, Committer B sees the PR, notices the label, opens it up, and merges without reviewing.</div></div></div></blockquote><div><br></div><div>If there is a Committer B, I think they should at least look at the PR to make sure it doesn’t have any hidden API changes, or introduce anything hostile.  </div><div><br></div><div>The point here is that getting the third person to look at a PR is proving quite difficult, and if the rest of the folks with the commit bit haven’t looked at a PR for a month, even after being warned, then silence indicates consent.  </div><div><br></div><div>Note that only folks with the commit bit can label PRs, so anyone flagging a PR like this is implicitly trusted to not be wreaking havoc, and not doing this for major changes.  Since we all curate our own PRs, I rather expect that the PR contributor will often be the person who adds the flag.  Whether we want to allow a self-merge at that point is up for debate, but if not, then folks need to get in the habit of looking at old flagged PRs and merging them.  </div><div><br></div><div>I think a huge problem we have is the LIFO default sort of the github PR queue, and that any PR not on the first page might as well be closed for all the attention it will get without constant nagging.  </div><div><br></div>Cheers,   Jody</div><div><br><blockquote type="cite"><div><div dir="ltr"><div>Does that capture a successful "single-review-merge" lifecycle?</div><div>-paul<br></div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Jan 15, 2019 at 9:57 AM 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>Hello,</div><div><br></div><div>Overall, I think this is a good idea, but I would like specifics on what "uncontroversial but uninteresting" PR mean. IMO, that should exclude any PR with API changes.</div><div><br></div><div>Cheers,</div><div>N<br></div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, 15 Jan 2019 at 06:00, Antony Lee <<a href="mailto:anntzer.lee@gmail.com" target="_blank">anntzer.lee@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>Hi all,</div><div><br></div><div>During the weekly dev call, I proposed to amend the PR merge rules, following an initial comment on Github [<a href="https://github.com/matplotlib/matplotlib/pull/13173#issuecomment-453921220" target="_blank">https://github.com/matplotlib/matplotlib/pull/13173#issuecomment-453921220</a>].  There was general agreement among the devs present (Tom, Hannah, Jody, and myself), so I'm putting it here for discussion.  The objective of this change is to prevent "uncontroversial but uninteresting" PRs from falling into oblivion, and try to decrease the size of the open PR stack.</div><div><br></div><div>The current rule is that a PR needs positive reviews (from two committers, and excluding the author if a committer) to be merged, except that doc-only PRs (docstrings, rst) only needs a single positive review.</div><div><br></div><div>I am proposing that, if a (non-doc) PR already has one positive review, but no activity on that PR has occurred for two weeks (exact time interval up to bikeshedding) [Jody suggests: and the PR has 100% code coverage], then a committer (either the first reviewer, or the author if a committer) can suggest that it be merged on the basis of that single review.  To do so, the "sponsor" should ping all developers (@matplotlib/developers) on that issue indicating that intent, and add a "single-review-merge" label on the PR (so that these PRs can easily be found).  Committers are encouraged to review the PR to accept and merge it or reject it or request changes on it; but they can also just indicate that they consider the PR sufficiently complex that a proper second review is needed before merging, or request an extension, etc.  To do so they should still leave a "reject" review, even if just saying "objecting to single-review merged; anyone can dismiss after a second review".  However, if within another two weeks, no committer voiced any objection (explicitly, i.e. by rejecting), then the PR can indeed be merged on the basis of that single review.</div><div><br></div><div>To avoid overwhelming the system, any committer can only "sponsor" a single PR at a time.</div><div><br></div><div>Thoughts?</div><div><br></div><div>Antony</div></div></div>
_______________________________________________<br>
Matplotlib-devel mailing list<br>
<a href="mailto:Matplotlib-devel@python.org" target="_blank">Matplotlib-devel@python.org</a><br>
<a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" rel="noreferrer" target="_blank">https://mail.python.org/mailman/listinfo/matplotlib-devel</a><br>
</blockquote></div>
_______________________________________________<br>
Matplotlib-devel mailing list<br>
<a href="mailto:Matplotlib-devel@python.org" target="_blank">Matplotlib-devel@python.org</a><br>
<a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" rel="noreferrer" target="_blank">https://mail.python.org/mailman/listinfo/matplotlib-devel</a><br>
</blockquote></div>
_______________________________________________<br>Matplotlib-devel mailing list<br><a href="mailto:Matplotlib-devel@python.org" target="_blank">Matplotlib-devel@python.org</a><br><a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" target="_blank">https://mail.python.org/mailman/listinfo/matplotlib-devel</a><br></div></blockquote></div><br><div>
<span class="gmail-m_-6427996525205526815gmail-m_2085301446449759380gmail-m_-5445635510087654823gmail-m_-867484032130323208Apple-style-span" style="border-collapse:separate;border-spacing:0px;color:rgb(0,0,0);font-family:"Lucida Sans Typewriter";font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div><span class="gmail-m_-6427996525205526815gmail-m_2085301446449759380gmail-m_-5445635510087654823gmail-m_-867484032130323208Apple-style-span" style="border-collapse:separate;border-spacing:0px;color:rgb(0,0,0);font-family:"Lucida Sans Typewriter";font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div>--</div><div>Jody Klymak    </div><div><a href="http://web.uvic.ca/~jklymak/" target="_blank">http://web.uvic.ca/~jklymak/</a></div><div><br class="gmail-m_-6427996525205526815gmail-m_2085301446449759380gmail-m_-5445635510087654823gmail-m_-867484032130323208khtml-block-placeholder"></div><div><br class="gmail-m_-6427996525205526815gmail-m_2085301446449759380gmail-m_-5445635510087654823gmail-m_-867484032130323208khtml-block-placeholder"></div><br class="gmail-m_-6427996525205526815gmail-m_2085301446449759380gmail-m_-5445635510087654823gmail-m_-867484032130323208Apple-interchange-newline"></span></div></span><br class="gmail-m_-6427996525205526815gmail-m_2085301446449759380gmail-m_-5445635510087654823gmail-m_-867484032130323208Apple-interchange-newline">
</div>
<br></div></blockquote></div>
_______________________________________________<br>
Matplotlib-devel mailing list<br>
<a href="mailto:Matplotlib-devel@python.org" target="_blank">Matplotlib-devel@python.org</a><br>
<a href="https://mail.python.org/mailman/listinfo/matplotlib-devel" rel="noreferrer" target="_blank">https://mail.python.org/mailman/listinfo/matplotlib-devel</a><br>
</blockquote></div>