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

[Antony Lee]

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/5f6c41f2/attachment.html>