10 Tips for Better Pull Requests

H. S. Teoh via Digitalmars-d digitalmars-d at puremagic.com
Fri Jan 16 09:40:11 PST 2015


On Fri, Jan 16, 2015 at 05:16:38PM +0000, Tobias Pankrath via Digitalmars-d wrote:
> On Friday, 16 January 2015 at 16:22:13 UTC, ketmar via Digitalmars-d wrote:
> >On Fri, 16 Jan 2015 08:10:50 -0800
> >Andrei Alexandrescu via Digitalmars-d <digitalmars-d at puremagic.com>
> >wrote:
[...]
> >>I think it would be great if we defined a simple policy for closing
> >>pull requests that are lingering. -- Andrei
> >
> >it sits in queue without any comments more than 20 days? reject and
> >close it.
> 
> Bad idea. Take for example this one of mine
> https://github.com/D-Programming-Language/phobos/pull/2793 that sits
> there for more than 20 days. I've addressed all concerns and now it's
> waiting for someone who feels responsible for std.container to pull
> it.
> 
> Now four things can happen:
> 
> 1. Someone pulls it. Fine.
> 2. Someone says, that after consideration this is not suitable for
> std.container. Fine.
> 3. Someone raises more QOI concerns. Fine.
> 4. Someone says, that the pull is rejected, because it was sitting
> there for more than 20 days. It would be my very last pull request.
> Period.
[...]

I think a reasonable policy is that if reviewers have left comments on
the PR but the submitter hasn't responded for >n days (whatever we
choose n to be), then it's a candidate for closing. The submitter is
free to resubmit it later. But if the submitter has addressed all
concerns raised and the reviewers are MIA, then it doesn't seem
reasonable to close the PR -- it would sound like "we're too lazy to
review your work, therefore we're rejecting it", which will turn people
away.

One issue is that Phobos has grown really big, and not all reviewers are
comfortable to review PRs in areas that they are not familiar with. At
least, I'm not comfortable doing that, or even when I do go out of my
way to do it, it's only possible when I have lots of free time to spend
in getting up to speed with that part of the code and then reviewing the
proposed changes. This issue is an argument for limiting the scope of
Phobos to something more manageable by the current pool of reviewers,
since otherwise large chunks of Phobos will bitrot and none of the
active reviewers would be able to do much about it. In the past, we have
taken this to be an argument rather for encouraging more participants,
but so far the number of active participants hasn't been able to keep up
with the sheer size of Phobos, which makes me think that we've bitten
off far more than we can chew.


T

-- 
In theory, there is no difference between theory and practice.


More information about the Digitalmars-d mailing list