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