[IPython-dev] Policy for closing pull requests

Brian Granger ellisonbg@gmail....
Mon Aug 13 22:14:31 CDT 2012


On Sun, Aug 12, 2012 at 11:36 PM, Fernando Perez <fperez.net@gmail.com> wrote:
> On Sun, Aug 12, 2012 at 9:53 PM, Brian Granger <ellisonbg@gmail.com> wrote:
>> I agree that Aaron's idea is important and I am +1 on implementing
>> this as you describe.  I just want their to be agreement on the
>> criteria we use to decide *when* we take such an action.  How do you
>> feel about the following:
>>
>> * When a PR has been reviewed and is waiting additional code and sits
>> in this state untouched for 1 month.
>
> Sounds more than reasonable to me.
>
>> * When the review process can't seem to reach a solid conclusion
>> because there are larger issues or technical details that have to be
>> worked out.  Once it reaches this point and sits for a month, we
>> should close the PR and open an issue to continue the larger
>> discussion.  I suppose if it becomes obvious that it is in this state
>> in a shorter amount of time, we could close it earlier.  The goal is
>> to have PRs reflect code that is actively moving forward towards a
>> merge.
>
> Agreed too, on all points.  When broader discussion is needed, the PR
> often ends up becoming a distraction, as it's too narrowly focused on
> a specific implementation.
>
>> * What do we do about PRs that sit for a month waiting for review?
>
> We flog ourselves with a nail-studded whip and we review them :)

I went back and forth of this one.  I mostly agree with you.  But our
busy schedules and the large number of PRs definitely makes it a
challenge to review everything promptly.

> Seriously, we should do our very best to avoid that from happening.
> And by 'we', I mean *anyone* who is interested in the future of
> IPython, not just those with commit rights!  One of the best way in
> which you can help our small team of core committers is by reviewing,
> testing and commenting on existing PRs.  It's perfectly possible for
> participants to refine a PR enough that by the time a core dev has a
> chance to look at it, all issues have been smoothed out and he can
> just merge it right away.
>
> So what I'd say is: if we find PRs in this kind of limbo, the right
> course of action is to come to the list with a plea for help.  We
> shouldn't ignore contributions from anyone for that long.  I would say
> that, even if it means delaying one of our pet PRs that may be in the
> queue, if this happens we should try really hard to prioritize those
> to help move them along, *especially* if they are from new
> contributors.  Who knows if we'd have Thomas, Matthias, Brad or others
> with us if the first time they tried to contribute it had taken months
> to get the first bit of feedback.
>
> In my mind, the priorities are:
>
> 1. PRs from new people.  Reply as fast as possible, even if it means
> just bouncing it back to them.  It's amazing how hard people will work
> for you on the internet *if you give them prompt and considered
> feedback*.  But if you let their contribution languish for weeks on
> end, then even a simple 'change this variable name to that and we'll
> merge' may get ignored, as they get disillusioned.
>
> 2. PRs from anyone else.
>
> 3. Open issues that have no attached code.

Yep.

>> We could go with a much simpler approach and say "regardless of the
>> reason if a PR hasn't been touched for a month, we close it and open
>> and issue."  A more uniform criteria might come across as less
>> personal.
>
> No, I don't think we need to establish such blind rules.  I'm always
> very skeptical of policies that try to eliminate the need for
> judgment, human intervention and participation.  Whether it's
> zero-tolerance policies in schools sending kids into trouble because
> they have a nail clipper ('a  weapon') or an aspirin ('dealing
> drugs'), or blind counting of journal impact factors as the sole
> metric for scientific advancement, policies that remove the element of
> judgment are, as far as I've seen, invariably worse than the problem
> they claim to solve.  So very, very strong -1 to this.

OK, I wasn't attached to this idea, just thought that is was a
possible alternative that was simple.

> I want IPython to be a project where, if we have *one* policy, it's
> that intelligent discussion comes *always* before policy.

Yes, but part of my goal is that we can come up with a guidelines for
closing PRs that are simple, and don't require long discussions.  If I
or someone else adheres to these guidelines are you OK with PRs being
closed without first running it by all interested parties?  To
summarize, here are the main points:

* PRs that haven't been reviewed for a month should prompt us to kick
each other into gear.  We should ping each other, the list and
specific people if necessary to get the review going.  The PR remains
open and we apologize to the author for our slowness.
* PRs that have been reviewed, but that sit for a month waiting for
code get closed.
* PRs that spawn larger discussions that are not resolved after a
month get closed.
* The person who closes an issue must open an issue that links to the
PR and tell the author that we welcome them to reopen the PR when they
start to work on it again.  If appropriate we can even give details of
the work that needs to be done.  But the tone must be one of
encouragement.  We can even site this guideline and our desire to keep
PRs moving.

Cheers,

Brian

> Cheers,
>
> f
> _______________________________________________
> IPython-dev mailing list
> IPython-dev@scipy.org
> http://mail.scipy.org/mailman/listinfo/ipython-dev



-- 
Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com


More information about the IPython-dev mailing list