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