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

Antony Lee antony.lee at institutoptique.fr
Wed Jan 16 12:53:16 EST 2019


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


More information about the Matplotlib-devel mailing list