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

[Antony Lee]

Agreed.

On Thu, Jan 17, 2019, 12:25 PM David Stansby <dstansby at gmail.com wrote:

> Apologies, my previous email was unnecessarily negative...
>
> I think the new system is a good idea, so here's where we seem to be. In
> terms of process:
>
> 1) PR opened
> 2) PR gets 1 positive review
> 3) 2 weeks after review the PR gets *no other significant activity, *and *qualifies
> for simpler review*
> 4) PR opener or reviewer 1 pings all Matplotlib devs
> 5) If 2 weeks after pinging no other significant activity has occurred, PR
> can be merged by PR opener or reviewer 1
>
> Does everyone agree on this? The bits I've put in bold still need to be
> defined; I don't have time now, but can provide a summary of these later.
>
> David
>
>
>
> On Thu, 17 Jan 2019 at 00:48, Antony Lee <antony.lee at institutoptique.fr>
> wrote:
>
>> On Wed, Jan 16, 2019 at 7:36 PM David Stansby <dstansby at gmail.com> wrote:
>>
>>> I think real improvements not getting in at the rate they are submitted
>>> is just how it works; if I started submitting a PR a day for the next 50
>>> days I would expect a good chunk of them sit there un-reviewed based on our
>>> current rate of merging things, and that's fine. The developers as a whole
>>> have a total reviewing rate, which is unpredictable and finite by the very
>>> nature of this being an open-source project where no-one is obliged to do
>>> anything. If we want to adjust reviewing rules to increase 'productivity'
>>> that seems fine to me, but I don't think we should be doing it with the end
>>> goal of being able to review at the rate that code is submitted (or any
>>> particular other rate).
>>>
>>
>> The end goal is not to set a specific rate, but it is indeed to try to
>> raise that rate up.  Obviously we could also throw our hands up and say
>> nothing can be done about it, but that seems a bit defeatist.
>>
>> Right now, I would bet that a lot of PRs go in with exactly two reviewers
>> having gone through them and no other reviewer having even skimmed them.
>> If anything, I believe that the system I propose will increase the number
>> of core-devs that'll have at least skimmed through the PR (all you need to
>> do, if you want to participate, is to check your github notifications once
>> every other week, whereas right now you'd actually need to pay attention to
>> each PR as it is being submitted), and it is quite likely that 1 in-depth
>> reviewer + 3-4 skim-throughs works as well, if not better, than 2
>> "in-depth" reviews (especially when the second one is sometimes "oh, that
>> looks fine and it's already been approved once, let's just commit it").
>>
>>
>>> re. large diff PRs, I find 100+ line PRs that are just copy and paste
>>> really hard to review - it's pretty dull for me trying to work out if code
>>> has been copy/pasted exactly without any errors (if there's an easier way
>>> to review stuff like this instead of checking every line please let me
>>> know!).
>>>
>>
>> Frankly, for a PR like that, I would just check that no function is
>> missing and that everything is test-covered (without changes to the tests),
>> and (in case you want to be sure) skim through the code to see that the PR
>> author has not added a sneaky vulnerability that allows them to take over
>> the user's computer... (In other words, I don't think a line-by-line check
>> matters for such cases.)
>>
>> Antony
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/matplotlib-devel/attachments/20190117/a13d03cf/attachment-0001.html>