• Seb (25/40) Mar 06 2016 Hey all,
• Jack Stouffer (18/26) Mar 06 2016 Unfortunately, this only works when the number of reviewers is
• Rikki Cattermole (6/6) Mar 06 2016 A couple years ago I fixed a bug in dmd.

Seb <seb wilzba.ch> writes:

Hey all,

I came by a couple of old PRs lately and while it's amusing that 
people like  JackStouffer celebrate their PR anniversaries, it's 
a bit sad for such an amazing project. By looking at those old 
PRs there is a common pattern - someone had a great idea for a 
feature, idea or fix and wants to get it merged. However after 
one or two week they usually move on and a lot of pinging on both 
sides is required.
In any case I think you already know this. Let me cite from the 
DLang progress report from H2 2015 [2].

 We fell short of our 2000 pull requests goal in H2 2015. We 
 have had only 1378 pull requests. This is a 10% decline 
 year-over-year compared to the same period in 2014. We 
 attribute this to the long time of reviewing many pull 
 requests, which works against a good contributor experience.

I do know that the time of reviewers is limited and voluntary, 
but I recently read a nice article [3] about Node's contribution 
policy. It is similar to your current policy:

 For non-trivial contributions, pull requests should sit for at 
 least 36 hours to ensure that contributors in other timezones 
 have time to review. Consideration should also be given to 
 weekends and other holiday periods to ensure active committers 
 all have reasonable time to become involved in the discussion 
 and review process if they wish.

However note the key difference:

 Rather than the default mode for a change to be rejected until 
 enough people sign off, we make the default for every change to 
 land. This puts the onus on reviewers to note exactly what 
 adjustments need to be made in order for it to land [4].

I really do like this idea. It could work if the review window is 
increased to maybe one week and everything without a "needs 
changes" tag (or "needs consensus" for major changes) is fine to 
merge then.

Before you start to protest - this is just an idea.

[1] https://github.com/D-Programming-Language/phobos/pull/3765
[2] http://wiki.dlang.org/Vision/2016H1
[3] 
https://medium.com/the-javascript-collection/healthy-open-source-967fa8be7951
[4] 
https://medium.com/the-javascript-collection/healthy-open-source-967fa8be7951#.loqfk3zf4

Jack Stouffer <jack jackstouffer.com> writes:

On Sunday, 6 March 2016 at 16:44:31 UTC, Seb wrote:
 Rather than the default mode for a change to be rejected until 
 enough people sign off, we make the default for every change 
 to land. This puts the onus on reviewers to note exactly what 
 adjustments need to be made in order for it to land [4].

 I really do like this idea. It could work if the review window 
 is increased to maybe one week and everything without a "needs 
 changes" tag (or "needs consensus" for major changes) is fine 
 to merge then.

Unfortunately, this only works when the number of reviewers is 
high and they review everything. Phobos contributions have about 
four regular reviews and about ten people who review on and off. 
I can't blame people for not reviewing things at once, because 
the things that I review take a while for me to get through 
because I find the process a little tedious (not a D/Phobos 
specific thing, it's just about looking at other's code). I 
usually allow for two weeks before I ping someone because of this.

The PR of mine you referred to had serious problems on it's first 
implementation and should not have been pulled in 36 hours. There 
was even a whole form discussion about it: 
http://forum.dlang.org/post/n0p4s3$5em$1 digitalmars.com

While I would love nothing more than for reviewing to go faster, 
I can't think of any mechanism to force reviews and contributors 
to respond faster to comments/make comments.

On a more positive note, trivial changes usually are merged 
within two days.

Rikki Cattermole <alphaglosined gmail.com> writes:

A couple years ago I fixed a bug in dmd.
After like a month it turned out it actually fixed multiple bugs.

It was not merged. Nobody reviewed it past fixing the commits as one.
Many months later another PR was made to fix only one of the bugs and 
that was merged.

Funny how things go. I have not tried to fix any since.