Proposal for pull request review process

[Daniel Murphy]

"Joseph Rushton Wakeling" <joseph.wakeling at webdrake.net> wrote in message 
news:mailman.289.1375044093.22075.digitalmars-d at puremagic.com...

> So, I'd propose that the review process include a way for the reviewers to
> indicate that a pull request is approved subject to passing the test 
> suite, and
> that it be obligatory for reviewers to provide such an indicator.
>

We already do this for dmd by commenting with "LGTM, waiting for green". 
Then if it goes green and the reviewer doesn't notice, the author can post 
something like "It's green now!" on the pull.  This works quite well, as 
another reviewer can pull without re-review if they notice first.

> Then, approved pull requests would go on a separate priority test queue 
> with
> "first in, first out" priority.  If the test suite passes, the pull 
> request is
> auto-merged, if it fails then it is removed from the queue and must be 
> re-approved.
>

The priority is based on how recently the pull request was modified.  If you 
find your pull request is somewhere at the bottom (or just not close enough 
to the top) you can simply rebase it on the latest master and re-push it. 
If you are not the author, a comment should be enough to ask the author to 
do it.

> Ideally it should be possible to distinguish actual test failures from 
> something
> going wrong with the test procedure itself (e.g. a test process not 
> spawning
> correctly), and in the latter case keeping the pull request in the 
> approved queue.
>

This would be nice, and should be fairly straighforward to implement if 
anybody feels like it.

> The result should be that reviewers can focus on _reviewing_, and don't 
> need to
> worry about waiting around for the test suite -- they can just approve in 
> the
> safe knowledge that the merge will happen (or not) depending on the test 
> results.
>

I haven't found this to be a problem with dmd.  Waiting for the test suite 
only seems to be a problem with really old pull requests, and they can be 
bumped easily with a rebase/push.

> Does this sound like something good to have, that would be feasible to 
> implement?
>

I'm not saying these things wouldn't be good to have, but I suspect the 
bottleneck is not enough people reviewing.