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

[David Stansby]

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/73ab9939/attachment.html>