http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/

I agree with pretty much everything in this article.

tl,dr:

"The more you make your reviewer work, the greater the risk is that your Pull Request will be rejected."

On Friday, 16 January 2015 at 04:20:37 UTC, Walter Bright wrote:
> http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/
>

Most are intuitive, but "10. Avoid thrashing" is worthy.

On Thu, Jan 15, 2015 at 08:20:32PM -0800, Walter Bright via Digitalmars-d wrote:
> http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/
> 
> I agree with pretty much everything in this article.
> 
> tl,dr:
> 
> "The more you make your reviewer work, the greater the risk is that your Pull Request will be rejected."

In the case of D, "the more you make your reviewer(s) work, the greater the risk is that your Pull Request will sit in the queue forever." :-P


T

-- 
When solving a problem, take care that you do not become part of the problem.

On 1/16/15 7:50 AM, H. S. Teoh via Digitalmars-d wrote:
> On Thu, Jan 15, 2015 at 08:20:32PM -0800, Walter Bright via Digitalmars-d wrote:
>> http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/
>>
>> I agree with pretty much everything in this article.
>>
>> tl,dr:
>>
>> "The more you make your reviewer work, the greater the risk is that
>> your Pull Request will be rejected."
>
> In the case of D, "the more you make your reviewer(s) work, the greater
> the risk is that your Pull Request will sit in the queue forever." :-P

I think it would be great if we defined a simple policy for closing pull requests that are lingering. -- Andrei

On Fri, 16 Jan 2015 08:10:50 -0800
Andrei Alexandrescu via Digitalmars-d <digitalmars-d@puremagic.com>
wrote:

> On 1/16/15 7:50 AM, H. S. Teoh via Digitalmars-d wrote:
> > On Thu, Jan 15, 2015 at 08:20:32PM -0800, Walter Bright via Digitalmars-d wrote:
> >> http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/
> >>
> >> I agree with pretty much everything in this article.
> >>
> >> tl,dr:
> >>
> >> "The more you make your reviewer work, the greater the risk is that your Pull Request will be rejected."
> >
> > In the case of D, "the more you make your reviewer(s) work, the greater the risk is that your Pull Request will sit in the queue forever." :-P
> 
> 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.

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@puremagic.com>
> wrote:
>
>> On 1/16/15 7:50 AM, H. S. Teoh via Digitalmars-d wrote:
>> > On Thu, Jan 15, 2015 at 08:20:32PM -0800, Walter Bright via Digitalmars-d wrote:
>> >> http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/
>> >>
>> >> I agree with pretty much everything in this article.
>> >>
>> >> tl,dr:
>> >>
>> >> "The more you make your reviewer work, the greater the risk is that
>> >> your Pull Request will be rejected."
>> >
>> > In the case of D, "the more you make your reviewer(s) work, the greater
>> > the risk is that your Pull Request will sit in the queue forever." :-P
>> 
>> 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.

On 1/16/15 9:16 AM, Tobias Pankrath 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@puremagic.com>
>> wrote:
>>
>>> On 1/16/15 7:50 AM, H. S. Teoh via Digitalmars-d wrote:
>>> > On Thu, Jan 15, 2015 at 08:20:32PM -0800, Walter Bright via >
>>> Digitalmars-d wrote:
>>> >> http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/
>>> >>
>>> >> I agree with pretty much everything in this article.
>>> >>
>>> >> tl,dr:
>>> >>
>>> >> "The more you make your reviewer work, the greater the risk >> is
>>> that
>>> >> your Pull Request will be rejected."
>>> >
>>> > In the case of D, "the more you make your reviewer(s) work, > the
>>> greater
>>> > the risk is that your Pull Request will sit in the queue >
>>> forever." :-P
>>>
>>> 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.

Thanks for raising attention to this. I didn't know about your pull request. A while ago I found my Inbox flooded by github-related messages so I directed them into a different folder. That has right now 13113 unread messages.

Your work is interesting and necessary. I shall destroy it soon :o).


Andrei

>> 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.
>
> Thanks for raising attention to this. I didn't know about your pull request. A while ago I found my Inbox flooded by github-related messages so I directed them into a different folder. That has right now 13113 unread messages.
>
> Your work is interesting and necessary. I shall destroy it soon :o).
>
>
> Andrei

That reply was not about my pull request specifically. Since it basically consists of two new files, it can stay there for months without generating any additional work for me. But it is a good counterexample to the »just close old stuff«-policy.

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

On Fri, 16 Jan 2015 17:16:38 +0000
Tobias Pankrath via Digitalmars-d <digitalmars-d@puremagic.com> 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@puremagic.com>
> > wrote:
> >
> >> On 1/16/15 7:50 AM, H. S. Teoh via Digitalmars-d wrote:
> >> > On Thu, Jan 15, 2015 at 08:20:32PM -0800, Walter Bright via Digitalmars-d wrote:
> >> >> http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/
> >> >>
> >> >> I agree with pretty much everything in this article.
> >> >>
> >> >> tl,dr:
> >> >>
> >> >> "The more you make your reviewer work, the greater the risk
> >> >> is that
> >> >> your Pull Request will be rejected."
> >> >
> >> > In the case of D, "the more you make your reviewer(s) work,
> >> > the greater
> >> > the risk is that your Pull Request will sit in the queue
> >> > forever." :-P
> >> 
> >> 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.
i still believe that this is a great idea. either it is significant enough to get some traction, or it doesn't needed. if upstream cannot upkeep with amount of changes, it's better to decreare incoming volume. "close after 20 days" is a "traffic shaper": if nobody cares for something withing 20 days (and nobody cares enough even to write "i'll busy now, but i plan to take a look at this withing next 10 days"), there is no sense to leave it rotting. what leaving it open does is stealing the original author's time which he dedicates to keep a PR "in shape" just in case that after another year somebody will look at it again.

so what devteam does with keeping it open is stealing other's time. and if enough time will pass withour author busy keeping PR up-to-date, it will eventually be closed as "unmergeable" anyway. 20 days of inactivity is enough to mark the thing as "not required right now".

> 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.
so you believe that it better left rotting without any sign of life for
monthes and only then someone will close it? so you'd better spend some
time in a hope that "maybe something will change" instead of clearly
see that nobody has enough time to look at it now, so it's better
be closed before it rots and stop eating your attention? nobody tells
that you cannot make a new PR with the same content if you really feels
that something must be done, along with NG posts.

the fact is that 20 days of inactivity is more than enough to tell that PR can be safely closed, as reviewers have more important things to do. they *always* will have more important things to do, so PR will be resheduled and resheduled and then it will rot and became inedible. or someone will stumble upon it on accident and yahoo! after seven and a half monthes it will be accepted... maybe.

see -- the best way to make someone react to your PR is to spam NG with requests to review it. or wait indefinitely.