www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - 10 Tips for Better Pull Requests

reply Walter Bright <newshound2 digitalmars.com> writes:
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."
Jan 15 2015
next sibling parent "eles" <eles eles.com> writes:
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.
Jan 16 2015
prev sibling parent reply "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
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.
Jan 16 2015
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
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
Jan 16 2015
parent reply ketmar via Digitalmars-d <digitalmars-d puremagic.com> writes:
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


=20
 I think it would be great if we defined a simple policy for closing pull=


=20

 requests that are lingering. -- Andrei


it sits in queue without any comments more than 20 days? reject and
close it.
Jan 16 2015
next sibling parent reply "Tobias Pankrath" <tobias pankrath.net> writes:
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.
Jan 16 2015
next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
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
Jan 16 2015
parent reply "Tobias Pankrath" <tobias pankrath.net> writes:
 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.
Jan 16 2015
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 1/16/15 9:41 AM, Tobias Pankrath wrote:

 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.


I agree that a hamfisted policy would do more harm than good. That's why 
it's so hard to define!

I'm thinking of something like: if there's $(legitimate) request for 
changes but the author is dormant for more than $(X) days, then close.

Andrei


Macros:

legitimate=?
X=?
Jan 16 2015
next sibling parent "Tobias Pankrath" <tobias pankrath.net> writes:
 I agree that a hamfisted policy would do more harm than good. 
 That's why it's so hard to define!

 I'm thinking of something like: if there's $(legitimate) 
 request for changes but the author is dormant for more than 
 $(X) days, then close.

 Andrei


 Macros:

 legitimate=?
 X=?


legitimate= consensual, if there is no consensus between phobos 
devs it cannot be pulled anyway. I'd think those are all 
changes/PR that are not tagged with »decision block«. I'd think 
even something like »fix your indentation« is legitimate.
X= 20 days, ask author if he's dead. If he does not respond in 10 
days, close and leave a comment to resubmit the pull request on 
resurrection.
Jan 16 2015
prev sibling next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 1/16/2015 9:49 AM, Andrei Alexandrescu wrote:

 I'm thinking of something like: if there's $(legitimate) request for changes
but
 the author is dormant for more than $(X) days, then close.


That's also a hamfisted policy. I've seen PR's that were good, but needed a bit 
of work, but the author disappeared. Sometimes, I've taken those over and 
finished them.

Arbitrarily closing them means they get lost forever.
Jan 16 2015
next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 1/16/15 11:23 AM, Walter Bright wrote:

 On 1/16/2015 9:49 AM, Andrei Alexandrescu wrote:

 I'm thinking of something like: if there's $(legitimate) request for
 changes but
 the author is dormant for more than $(X) days, then close.


 That's also a hamfisted policy. I've seen PR's that were good, but
 needed a bit of work, but the author disappeared. Sometimes, I've taken
 those over and finished them.

 Arbitrarily closing them means they get lost forever.


Look for a champion after $(X) days? It looks like once a pull request 
is open it's impossible to close it. There's got to be some garbage 
collection somehow :o). -- Andrei
Jan 16 2015
next sibling parent Walter Bright <newshound2 digitalmars.com> writes:
On 1/16/2015 12:44 PM, Andrei Alexandrescu wrote:

 On 1/16/15 11:23 AM, Walter Bright wrote:

 On 1/16/2015 9:49 AM, Andrei Alexandrescu wrote:

 I'm thinking of something like: if there's $(legitimate) request for
 changes but
 the author is dormant for more than $(X) days, then close.


 That's also a hamfisted policy. I've seen PR's that were good, but
 needed a bit of work, but the author disappeared. Sometimes, I've taken
 those over and finished them.

 Arbitrarily closing them means they get lost forever.


 Look for a champion after $(X) days? It looks like once a pull request is open
 it's impossible to close it. There's got to be some garbage collection somehow
 :o). -- Andrei


Should we also simply close open Bugzilla issues after X days? I don't think so.

Sometimes we just aren't 'ready' for a particular PR, but it is still valuable, 
such as the concurrent GC one. Deleting it (what closing really is) is not a 
solution.

I'd like to revisit the assumption that aged PRs is so bad that they must be 
removed. It reminds me of how the government got Amtrak trains to run on time
by 
redefining on time from +-5 min to +-30 min.

I do agree that often good PRs get overlooked, and that's shameful. Deleting 
them is not the answer. Old PRs are not garbage.
Jan 16 2015
prev sibling parent Jacob Carlborg <doob me.com> writes:
On 2015-01-16 21:44, Andrei Alexandrescu wrote:


 Look for a champion after $(X) days? It looks like once a pull request
 is open it's impossible to close it. There's got to be some garbage
 collection somehow :o). -- Andrei


It's always possible to add a label to the pull request.

-- 
/Jacob Carlborg
Jan 17 2015
prev sibling next sibling parent reply "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Friday, 16 January 2015 at 19:23:06 UTC, Walter Bright wrote:

 On 1/16/2015 9:49 AM, Andrei Alexandrescu wrote:

 I'm thinking of something like: if there's $(legitimate) 
 request for changes but
 the author is dormant for more than $(X) days, then close.


 That's also a hamfisted policy. I've seen PR's that were good, 
 but needed a bit of work, but the author disappeared. 
 Sometimes, I've taken those over and finished them.

 Arbitrarily closing them means they get lost forever.


Add an "abandoned" label, and assign it when closing? Then they 
won't get lost, but also won't clutter the list.
Jan 16 2015
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 1/16/2015 12:48 PM, Vladimir Panteleev wrote:

 On Friday, 16 January 2015 at 19:23:06 UTC, Walter Bright wrote:

 On 1/16/2015 9:49 AM, Andrei Alexandrescu wrote:

 I'm thinking of something like: if there's $(legitimate) request for changes
but
 the author is dormant for more than $(X) days, then close.


 That's also a hamfisted policy. I've seen PR's that were good, but needed a
 bit of work, but the author disappeared. Sometimes, I've taken those over and
 finished them.

 Arbitrarily closing them means they get lost forever.


 Add an "abandoned" label, and assign it when closing? Then they won't get lost,
 but also won't clutter the list.


What should happen is the list of PRs should be sorted on a basis of 
most-recently-touched-is-first. The older PRs then naturally fade to the 'next' 
pages. 'clutter' is not a problem, unless we decide that number of open PRs is
a 
proxy for quality of the project.

We should be very careful about working metrics like "number of open PRs".
Focus 
on such can generate an illusion of progress, and actually make things worse.

I've worked at companies that would rate engineers based on the "bug count". 
That ended very badly, it was so bad it was comical, how working that number 
actually wrecked the quality of the product. I've seen similar disasters with 
use of metrics on inventory minimization, and others. Ever since it has made me 
deeply suspicious about basing quality on such numbers.
Jan 16 2015
next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 1/16/15 2:17 PM, Walter Bright wrote:

 I've worked at companies that would rate engineers based on the "bug
 count". That ended very badly, it was so bad it was comical, how working
 that number actually wrecked the quality of the product. I've seen
 similar disasters with use of metrics on inventory minimization, and
 others. Ever since it has made me deeply suspicious about basing quality
 on such numbers.


One anecdote is what it is. There is a lot of value in informative 
proxies. -- Andrei
Jan 16 2015
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 1/16/2015 2:28 PM, Andrei Alexandrescu wrote:

 On 1/16/15 2:17 PM, Walter Bright wrote:

 I've worked at companies that would rate engineers based on the "bug
 count". That ended very badly, it was so bad it was comical, how working
 that number actually wrecked the quality of the product. I've seen
 similar disasters with use of metrics on inventory minimization, and
 others. Ever since it has made me deeply suspicious about basing quality
 on such numbers.


 One anecdote is what it is. There is a lot of value in informative proxies. --
 Andrei


Informative is fine. Basing decisions on metrics unleavened by contextual 
judgement isn't going to work well.

It isn't just one metric. I've personally seen it multiple times with various 
metrics, and regularly read in the news about counterproductive results
obtained 
by using metrics absent judgement. The "zero tolerance" policies schools have 
are a stellar example, where students get punished for chewing a pizza into the 
shape of a gun.

 
http://www.thetruthaboutguns.com/2011/12/daniel-zimmerman/zero-tolerance-idiot-of-the-day-principal-steve-luker/

Want more? Consider "work to rule" tactics used by unions.

   http://en.wikipedia.org/wiki/Work-to-rule

I agree we have a problem with good PRs left twisting in the wind. Deleting
them 
simply because they are old is worse.
Jan 16 2015
parent "Laeeth Isharc" <Laeeth.nospam nospam-laeeth.com> writes:
 Informative is fine. Basing decisions on metrics unleavened by 
 contextual judgement isn't going to work well.

 It isn't just one metric. I've personally seen it multiple 
 times with various metrics, and regularly read in the news 
 about counterproductive results obtained by using metrics 
 absent judgement. The "zero tolerance" policies schools have 
 are a stellar example, where students get punished for chewing 
 a pizza into the shape of a gun.


+1.  Dr Iain MacGilchrist at All Souls, Oxford, has written a 
whole book about the mindset behind mistaking abstracted 
representations of the world for the world itself.

Would it be worth considering a simple portal where people can 
vote on pull requests with a comment, and then at least one can 
see them ranked by what people find important?  This is 
susceptible to all the usual problems of democracy, but it might 
be an improvement.


Laeeth
Jan 17 2015
prev sibling parent reply "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Fri, Jan 16, 2015 at 02:17:02PM -0800, Walter Bright via Digitalmars-d wrote:

 On 1/16/2015 12:48 PM, Vladimir Panteleev wrote:


[...]

Add an "abandoned" label, and assign it when closing? Then they won't
get lost, but also won't clutter the list.


 
 What should happen is the list of PRs should be sorted on a basis of
 most-recently-touched-is-first. The older PRs then naturally fade to
 the 'next' pages. 'clutter' is not a problem, unless we decide that
 number of open PRs is a proxy for quality of the project.


Github already has a feature for sorting the PR list by most recently
updated, least recently updated, oldest, newest, etc., etc..

It's just a matter of setting the *default* sorting order. I don't know
if github supports that, but for a long time now I've been wishing that
the PR page should default to "most recently update" instead of
"newest", as it is now.



 We should be very careful about working metrics like "number of open
 PRs".  Focus on such can generate an illusion of progress, and
 actually make things worse.
 
 I've worked at companies that would rate engineers based on the "bug
 count".  That ended very badly, it was so bad it was comical, how
 working that number actually wrecked the quality of the product. I've
 seen similar disasters with use of metrics on inventory minimization,
 and others. Ever since it has made me deeply suspicious about basing
 quality on such numbers.


Some companies, in the name of reining in the number of open bugs,
resort to closing "inactive" (but nonetheless valid) bugs after a given
amount of time. Sure, it reduces some integer on somebody's statistics
page, but did it increase the quality of the product? Nope. Did it
reduce the amount of work needed? Nope, on the contrary, it *increased*
it, since later on some other customer inevitably runs into the same
problem whose bug report got closed down, so now we have extra overhead
for (re)processing the "new" bug, and repeating the research that has
been buried in the dusts of bugtracker history in order to get back up
to speed with the problem.

Perhaps what *should* have happened, is that somebody's statistics page
should have included a filter that limited the scope of the query to the
last X months for some suitable value of X, rather than count *all* bugs
from the beginning of time until now.


T

-- 
Never step over a puddle, always step around it. Chances are that whatever made
it is still dripping.
Jan 16 2015
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 1/16/2015 2:59 PM, H. S. Teoh via Digitalmars-d wrote:

 Github already has a feature for sorting the PR list by most recently
 updated, least recently updated, oldest, newest, etc., etc..

 It's just a matter of setting the *default* sorting order. I don't know
 if github supports that, but for a long time now I've been wishing that
 the PR page should default to "most recently update" instead of
 "newest", as it is now.


Yes, I'd like to change the default.



 Some companies, in the name of reining in the number of open bugs,
 resort to closing "inactive" (but nonetheless valid) bugs after a given
 amount of time. Sure, it reduces some integer on somebody's statistics
 page, but did it increase the quality of the product? Nope. Did it
 reduce the amount of work needed? Nope, on the contrary, it *increased*
 it, since later on some other customer inevitably runs into the same
 problem whose bug report got closed down, so now we have extra overhead
 for (re)processing the "new" bug, and repeating the research that has
 been buried in the dusts of bugtracker history in order to get back up
 to speed with the problem.


Yup. I fail to see how automatically closing PRs after X days can possibly 
result in an improved quality of the product.

And even worse, it can result in someone unwittingly doing work to submit a new 
PR that solves the same problem. We can't afford people wasting effort like
that.
Jan 16 2015
parent reply "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Fri, Jan 16, 2015 at 03:33:00PM -0800, Walter Bright via Digitalmars-d wrote:

 On 1/16/2015 2:59 PM, H. S. Teoh via Digitalmars-d wrote:

Github already has a feature for sorting the PR list by most recently
updated, least recently updated, oldest, newest, etc., etc..

It's just a matter of setting the *default* sorting order. I don't
know if github supports that, but for a long time now I've been
wishing that the PR page should default to "most recently update"
instead of "newest", as it is now.


 
 Yes, I'd like to change the default.


Does github have that option? If so, can somebody who has the rights
make this change?



Some companies, in the name of reining in the number of open bugs,
resort to closing "inactive" (but nonetheless valid) bugs after a
given amount of time. Sure, it reduces some integer on somebody's
statistics page, but did it increase the quality of the product?
Nope. Did it reduce the amount of work needed? Nope, on the contrary,
it *increased* it, since later on some other customer inevitably runs
into the same problem whose bug report got closed down, so now we
have extra overhead for (re)processing the "new" bug, and repeating
the research that has been buried in the dusts of bugtracker history
in order to get back up to speed with the problem.


 
 Yup. I fail to see how automatically closing PRs after X days can
 possibly result in an improved quality of the product.


It may not directly impact the quality of the product, but it *could*
affect morale (potential contributor looks at the PR list, sees it's
90+, and feels that it's unlikely his contributions will ever get
accepted, so gives up before even trying), which in the long run does
have impact on product quality. Unfortunately, contributors are not
machines, so such emotional factors can and do make a difference.



 And even worse, it can result in someone unwittingly doing work to
 submit a new PR that solves the same problem. We can't afford people
 wasting effort like that.


Actually, we're already at risk of that. If the PR list is 5-6 pages
long, how likely is it that a potential contributor will scan through
all of them before deciding that his work isn't duplicated work? In
theory, everyone *should*, but I highly doubt anyone submitting a PR
actually reads beyond the first page (if even that!).


T

-- 
People tell me that I'm paranoid, but they're just out to get me.
Jan 16 2015
next sibling parent "Zach the Mystic" <reachzach gggmail.com> writes:
On Friday, 16 January 2015 at 23:51:40 UTC, H. S. Teoh via 
Digitalmars-d wrote:

 It may not directly impact the quality of the product, but it 
 *could* affect morale (potential contributor looks at the PR
 list, sees it's 90+, and feels that it's unlikely his 
 contributions
 will ever get accepted, so gives up before even trying), which
 in the long run does have impact on product quality.
 Unfortunately, contributors are not machines, so such emotional 
 factors can and do make a difference.


I agree, H.S... nurturing the contributor experience should be 
the only consideration. But I doubt people care about numbers. 
Instead, they care that *their* suggestion, *their* improvement 
is responded to. I'd venture to say that nothing matters more 
than knowing that one has been heard.
Jan 16 2015
prev sibling parent Jacob Carlborg <doob me.com> writes:
On 2015-01-17 00:49, H. S. Teoh via Digitalmars-d wrote:


 Does github have that option? If so, can somebody who has the rights
 make this change?


Not as far as I can see. The settings page for a project doesn't contain 
much.

-- 
/Jacob Carlborg
Jan 17 2015
prev sibling next sibling parent ketmar via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Fri, 16 Jan 2015 11:23:02 -0800
Walter Bright via Digitalmars-d <digitalmars-d puremagic.com> wrote:


 On 1/16/2015 9:49 AM, Andrei Alexandrescu wrote:

 I'm thinking of something like: if there's $(legitimate) request for ch=




anges but

 the author is dormant for more than $(X) days, then close.


=20
 That's also a hamfisted policy. I've seen PR's that were good, but needed=


 a bit=20

 of work, but the author disappeared. Sometimes, I've taken those over and=


=20

 finished them.
=20
 Arbitrarily closing them means they get lost forever.


that's why there will ALWAYS be alot of items in queue and it will not
be processed at any sane rate. add ten more people to process the PR
queue, and... there will be just more and more unprocessed PRs.

it's cause you're looking at that queue like it's a backpack full of
things that can lay there forever. sometimes somebody can pull some
thing out of backpack and work on it. but backpack will accumulate more
and more things over the time.

this will continue unless you realise that PR queue is not a backpack
full of possible cool things. it's a queue of things that *must* *be*
*processed* *within* *short* *time*. if it took more than ten days to
simply react on the thing -- throw it out, it just cluttering the queue.

this is one of the things that "github culture" gets very-very wrong. a
proper place to accumulate requests and code is bugzilla. and PR queue
is a queue for things that finally moved to "merging stage". i.e. for
things that must be reviewed and merged (or rejected) ASAP.

it doesn't matter how hard somebody will try to keep that queue
"manageable", it will be oveflown with requests. the only way to manage
it is to turn it from backpack to "ASAP-queue". so i repeat my point: if
something sits there for twenty days (let's add ten days) without any
motion -- remove it from the queue. either this, or it always be
cluttered.
Jan 16 2015
prev sibling parent Jacob Carlborg <doob me.com> writes:
On 2015-01-16 20:23, Walter Bright wrote:


 Arbitrarily closing them means they get lost forever.


How so?

-- 
/Jacob Carlborg
Jan 17 2015
prev sibling parent Jacob Carlborg <doob me.com> writes:
On 2015-01-16 18:49, Andrei Alexandrescu wrote:


 I agree that a hamfisted policy would do more harm than good. That's why
 it's so hard to define!

 I'm thinking of something like: if there's $(legitimate) request for
 changes but the author is dormant for more than $(X) days, then close.


Does the opposite apply? If no reviewer has comment anything in $(X) day 
the pull request is merged ;)

-- 
/Jacob Carlborg
Jan 17 2015
prev sibling next sibling parent "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
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.
Jan 16 2015
prev sibling parent ketmar via Digitalmars-d <digitalmars-d puremagic.com> writes:
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=20
 Digitalmars-d wrote:

 On Fri, 16 Jan 2015 08:10:50 -0800
 Andrei Alexandrescu via Digitalmars-d=20
 <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=20
 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=20
 is that
 your Pull Request will be rejected."


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


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


 it sits in queue without any comments more than 20 days? reject=20
 and
 close it.


=20
 Bad idea. Take for example this one of mine=20
 https://github.com/D-Programming-Language/phobos/pull/2793 that=20
 sits there for more than 20 days. I've addressed all concerns and=20
 now it's waiting for someone who feels responsible for=20
 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=20
 sitting there for more than 20 days. It would be my very last=20
 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.
Jan 16 2015
prev sibling parent reply "deadalnix" <deadalnix gmail.com> writes:
On Friday, 16 January 2015 at 16:22:13 UTC, ketmar via 
Digitalmars-d wrote:

 it sits in queue without any comments more than 20 days? reject 
 and
 close it.


It is better to have some kind of bot that comment on the PR 
after a while. Like "hey, this PR is hanging, can someone make 
thing go forward or I'll close in 2 more month". That generate 
activity on the PR and is often a wake up call for people.
Jan 16 2015
next sibling parent "aldanor" <i.s.smirnov gmail.com> writes:
On Friday, 16 January 2015 at 18:50:29 UTC, deadalnix wrote:

 On Friday, 16 January 2015 at 16:22:13 UTC, ketmar via 
 Digitalmars-d wrote:

 it sits in queue without any comments more than 20 days? 
 reject and
 close it.


 It is better to have some kind of bot that comment on the PR 
 after a while. Like "hey, this PR is hanging, can someone make 
 thing go forward or I'll close in 2 more month". That generate 
 activity on the PR and is often a wake up call for people.


On a relevant sidenote: this is a GitHub buildbot that manages 
Rust's main repo, PR approvals and all that: 
https://github.com/graydon/bors

IMO that seems to work quite well for Rust and lowers the 
administrative burden on reviewers.
Jan 16 2015
prev sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2015-01-16 19:50, deadalnix wrote:


 It is better to have some kind of bot that comment on the PR after a
 while. Like "hey, this PR is hanging, can someone make thing go forward
 or I'll close in 2 more month". That generate activity on the PR and is
 often a wake up call for people.


Ruby on Rails has something like that for their project. Although 
there's a huge unfairness if a reviewer never replies. Just because a 
reviewer doesn't reply doesn't mean the problem (i.e. a bug) goes away. 
Yes, they're using this for issues.

-- 
/Jacob Carlborg
Jan 17 2015
parent "aldanor" <i.s.smirnov gmail.com> writes:
On Saturday, 17 January 2015 at 11:52:03 UTC, Jacob Carlborg 
wrote:

 On 2015-01-16 19:50, deadalnix wrote:


 It is better to have some kind of bot that comment on the PR 
 after a
 while. Like "hey, this PR is hanging, can someone make thing 
 go forward
 or I'll close in 2 more month". That generate activity on the 
 PR and is
 often a wake up call for people.


 Ruby on Rails has something like that for their project. 
 Although there's a huge unfairness if a reviewer never replies. 
 Just because a reviewer doesn't reply doesn't mean the problem 
 (i.e. a bug) goes away. Yes, they're using this for issues.


The whole thing would be much easier if github issues were used 
instead of bugzilla -- for one thing, it would automatically 
mention it in a pull request (or an issue) if it was mentioned 
anywhere in another issue / pull request which makes browsing and 
figuring what relates to what much easier (instead of having to 
search bugzilla / forum / pull requests here and having three 
different places to register things at). Just my 2 cents...
Jan 17 2015