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

[David Stansby]

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).

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!).

David

On Wed, 16 Jan 2019 at 17:54, Antony Lee <antony.lee at institutoptique.fr>
wrote:

>
>
> On Wed, Jan 16, 2019 at 6:26 PM Nelle Varoquaux <nelle.varoquaux at gmail.com>
> wrote:
>
>>
>> I didn't mean to imply that you had a secret agenda in changing the API
>> in a backward incompatible way. But mistakes happen, and looking for
>> backward incompatibility has not been a top priority for some core
>> developers recently, and  over time it creates a culture where looking
>> carefully for backward incompatibility is not part of our reviewing
>> process. Having two core developers review changes limits the chance of
>> such pull requests being merged with backward incompatibility. I also think
>> that every API change, including the addition of keywords, needs to be
>> considered carefully. Part of the issues we are having in terms of
>> maintenance are that too many functions are public and many of those don't
>> have a consistent API with the rest of the library.
>>
>
> Again, you may note that I have fairly few PRs that introduce new
> functionality; most of the API changes I PR'd actually deprecate and later
> remove functions that "should not have been public" (I have sort of given
> up on making things consistent in an backcompatible manner).  So in that
> respect I am mostly trying to improve maintanability...
>
>
>> To summarize the points that have been agreed upon and the points that
>> still need to be discussed. I've removed the one about documentation, but
>> if we accept API changes in this system, it should probably added back as a
>> point to be discussed.
>>
>> Agreed upon:
>> - Has not been commented on by more than one core developer;
>> - Doesn't fix a release-critical bug;
>> - Is fully tested.
>>
>> Still needs to be discussed:
>> 1. Doesn't change the public API;
>> 2. Doesn't add or remove a functionality;
>> 3. Doesn't add or remove a dependency;
>> 4. Is less than XX lines of code. (Long PR are harder to review, and thus
>> the likelihood of one core dev not noticing a problem is higher). Threshold
>> needs to be defined.
>>
>> I'd also like to add:
>> 5. All CI passes (because we have, in the past, ignored some errors on
>> Travis when we felt tests were flaky)
>>
>
> Agreed on 5.
>
>
>> I maintain that any pull requests that add or remove a functionality
>> should be reviewed by 2 core developers. I guess our BDFL may have to make
>> the final call on that one :)
>>
>
> Happy to let Tom decide, but again I would say that "add or remove a
> functionality" is not always an objective feature either (e.g., does the
> LocationEvent-integer PR qualify?).
>
>
>> My argument for 4. is that long pull requests are harder to review, even
>> if they are trivial (there are studies on this.). It is thus easier to make
>> a mistake in reviewing.
>>
>
> As I already mentioned, there have been cases of 2-line PRs (+ 20 lines of
> *new* tests) making backwards incompatible changes, whereas some 100+ lines
> PR are essentially copy-paste trivialities, so I really don't buy the
> argument.
>
>
>> In terms of number of pull request per core dev, it is just that more
>> pull requests dilute our effort in reviewing. The more someone has pull
>> request open, the less likely they are to be reviewed and the more likely
>> that developer is to "forget" about them. I don't think we should have a
>> rule on the number of pull request open per person, but on the different
>> project I contribute to, I have a rule of thumb to not have more than 5
>> pull requests opened. It forces me to finish up the work on the pull
>> requests I have opened before working on something else.
>>
>
> Frankly it's not clear what "finish up the work" is supposed to mean.  If
> you look at the PRs I've mentioned above they have been merged with (close
> to) no changes (except for repeated rebases on master...) after the
> 6-to-12-month review period, so essentially all the work was done from the
> beginning.
>
>
>> As for David's point, it just happens that some collaborators and I are
>> working on understanding factors for newcomer retention and developer
>> burnout. We've got preliminary results that we are not ready to share but
>> we are hoping that it'll help on that point.
>>
>
> Happy to hear anything that could be done to help on that point.  But I
> guess I'll take advantage of this to reply to Tom's remark as well
>
> [Tom] I want to push back strongly on the notion that "did not review -->
>> does not care".  As as already been discussed above there are many other
>> reasons someone does not comment (for example "I _do_ care about this, but
>> I {do not have time, am too tired and cranky, ...} to properly review this
>> now").
>
>
> I'm not pointing fingers at anyone not doing their review share, and once
> again totally agree with David's point regarding no one being paid.
> However, you have to realize that the consequences of insufficient review
> volume is that real improvements are not getting in (either because the PRs
> stall out, or (in my case) because I don't even bother PR'ing them anymore
> and just live-patch matplotlib locally), and saying "it's no-one's fault"
> (which is true) is not going to help.
>
> Cheers,
>> N
>>
>
> Antony
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/matplotlib-devel/attachments/20190116/7ed55f02/attachment-0001.html>