[Matplotlib-devel] Proposal to amend the PR merge rules
Thomas Caswell
tcaswell at gmail.com
Tue Jan 15 18:18:52 EST 2019
I think a distinction between "no one cares" and "no one has bandwidth"
needs to be made.
If you look at the 'pulse' page, we merging 100+ PRs a month (
https://github.com/matplotlib/matplotlib/pulse/monthly) so reviews are
happening, the issue is just that some are falling through the cracks.
Tom
On Tue, Jan 15, 2019 at 4:45 PM Antony Lee <antony.lee at institutoptique.fr>
wrote:
> Eric,
> 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.
> 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
> https://github.com/matplotlib/matplotlib/pull/12928, 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)
> Antony
>
> On Tue, Jan 15, 2019 at 10:36 PM Eric Firing <efiring at hawaii.edu> wrote:
>
>> Antony,
>>
>> Your examples illustrate that the problem is often that we don't have
>> enough people who understand some areas, like fonts and pdf/ps file
>> formats, to get 2 thorough and knowledgeable reviews in a reasonable time.
>>
>> I'm fine with your proposed rule. It should help at least a little bit.
>>
>> Eric
>>
>>
>> On 2019/01/15 9:11 AM, Antony Lee wrote:
>> > 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.
>> >
>> > - https://github.com/matplotlib/matplotlib/pull/9787 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:
>> > * adding "ttc" as an extension that should be treated like
>> "ttf"/"otf",
>> > * adding error handling to pdf/ps to signal these formats are not
>> > supported there,
>> > * tests.
>> > 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.
>> >
>> > - https://github.com/matplotlib/matplotlib/pull/12472 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.
>> >
>> > - https://github.com/matplotlib/matplotlib/pull/9867 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.
>> >
>> > -----
>> >
>> > As for the specifics (as I see it):
>> > - 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).
>> > - 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.
>> > - 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).
>> >
>> > I would not exclude all API changes from the process. For example,
>> >
>> https://github.com/matplotlib/matplotlib/pull/13173/commits/e90d264f3e5a263ef57243a2af86b46ab74ccc16
>> > 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).
>> >
>> > Antony
>> >
>> > On Tue, Jan 15, 2019 at 7:46 PM Nelle Varoquaux
>> > <nelle.varoquaux at gmail.com <mailto:nelle.varoquaux at gmail.com>> wrote:
>> >
>> > 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.
>> >
>> >
>> > On Tue, 15 Jan 2019 at 10:37, Jody Klymak <jklymak at uvic.ca
>> > <mailto:jklymak at uvic.ca>> wrote:
>> >
>> >
>> >
>> >> On 15 Jan 2019, at 10:20, Paul Hobson <pmhobson at gmail.com
>> >> <mailto:pmhobson at gmail.com>> wrote:
>> >>
>> >> I think this would be a good policy as well. Can I get some
>> >> clarification on the flow? The way I understand it:
>> >>
>> >> 1) Someone (committer, contributor, or first-timer) opens a
>> >> simple PR
>> >> 2) Committer A reviews it, adds a "single-review-merge" label,
>> >> then digitally walks away
>> >> 3) No less than two weeks later, Committer B sees the PR,
>> >> notices the label, opens it up, and merges without reviewing.
>> >
>> > 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.
>> >
>> > 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.
>> >
>> > 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.
>> >
>> > 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.
>> >
>> > Cheers, Jody
>> >
>> >> Does that capture a successful "single-review-merge" lifecycle?
>> >> -paul
>> >>
>> >> On Tue, Jan 15, 2019 at 9:57 AM Nelle Varoquaux
>> >> <nelle.varoquaux at gmail.com <mailto:nelle.varoquaux at gmail.com>>
>> >> wrote:
>> >>
>> >> Hello,
>> >>
>> >> 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.
>> >>
>> >> Cheers,
>> >> N
>> >>
>> >> On Tue, 15 Jan 2019 at 06:00, Antony Lee
>> >> <anntzer.lee at gmail.com <mailto:anntzer.lee at gmail.com>>
>> wrote:
>> >>
>> >> Hi all,
>> >>
>> >> During the weekly dev call, I proposed to amend the PR
>> >> merge rules, following an initial comment on Github
>> >> [
>> https://github.com/matplotlib/matplotlib/pull/13173#issuecomment-453921220].
>>
>> >> 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.
>> >>
>> >> 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.
>> >>
>> >> 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.
>> >>
>> >> To avoid overwhelming the system, any committer can
>> >> only "sponsor" a single PR at a time.
>> >>
>> >> Thoughts?
>> >>
>> >> Antony
>> >> _______________________________________________
>> >> Matplotlib-devel mailing list
>> >> Matplotlib-devel at python.org
>> >> <mailto:Matplotlib-devel at python.org>
>> >>
>> https://mail.python.org/mailman/listinfo/matplotlib-devel
>> >>
>> >> _______________________________________________
>> >> Matplotlib-devel mailing list
>> >> Matplotlib-devel at python.org
>> >> <mailto:Matplotlib-devel at python.org>
>> >> https://mail.python.org/mailman/listinfo/matplotlib-devel
>> >>
>> >> _______________________________________________
>> >> Matplotlib-devel mailing list
>> >> Matplotlib-devel at python.org <mailto:
>> Matplotlib-devel at python.org>
>> >> https://mail.python.org/mailman/listinfo/matplotlib-devel
>> >
>> > --
>> > Jody Klymak
>> > http://web.uvic.ca/~jklymak/
>> >
>> >
>> >
>> >
>> >
>> > _______________________________________________
>> > Matplotlib-devel mailing list
>> > Matplotlib-devel at python.org <mailto:Matplotlib-devel at python.org>
>> > https://mail.python.org/mailman/listinfo/matplotlib-devel
>> >
>> >
>> > _______________________________________________
>> > Matplotlib-devel mailing list
>> > Matplotlib-devel at python.org
>> > https://mail.python.org/mailman/listinfo/matplotlib-devel
>> >
>>
>> _______________________________________________
>> Matplotlib-devel mailing list
>> Matplotlib-devel at python.org
>> https://mail.python.org/mailman/listinfo/matplotlib-devel
>>
> _______________________________________________
> Matplotlib-devel mailing list
> Matplotlib-devel at python.org
> https://mail.python.org/mailman/listinfo/matplotlib-devel
>
--
Thomas Caswell
tcaswell at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/matplotlib-devel/attachments/20190115/7d1895f1/attachment-0001.html>
More information about the Matplotlib-devel
mailing list