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 1Does 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>