[Matplotlib-devel] Proposal to amend the PR merge rules

Nelle Varoquaux nelle.varoquaux at gmail.com
Tue Jan 15 13:45:45 EST 2019


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> wrote:

>
>
> On 15 Jan 2019, at 10:20, Paul Hobson <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>
> 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> 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
>>> 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
>
>
> --
> Jody Klymak
> http://web.uvic.ca/~jklymak/
>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/matplotlib-devel/attachments/20190115/41a204e1/attachment.html>


More information about the Matplotlib-devel mailing list