On Tuesday, 23 July 2013 at 19:24:10 UTC, Andrei Alexandrescu
wrote:
I'm very surprised by your outlook. My perception is that the
long queue of pending pull requests not being reviewed is the
single most important bottleneck at this point in history in
the path of D. By my estimates I think we'd improve the speed
of D's development by at least one third if we solve this one
issue. There's no other issue offering so much impact.
I agree it's the major bottleneck but disagree slightly about the
details.
My recent experience has been that my Phobos pull requests get
_reviewed_ quite quickly but then it may take quite some time to
actually get merged. Confusion can be added because the reviewers
don't always indicate explicit approval of the code, so the
submitter can be sitting in limbo not knowing if the lack of
merge is down to the code still being inadequate or just the
reviewer not getting round to merging it yet.
The latter kind of delay tends to result from the situation where
the reviewer is waiting for the test suite to pass. Because
there's no option to auto-merge on pass, and no alert to
reviewers that a pull request has passed testing, it's easy to
miss windows of opportunity to merge. This only has to happen a
few times for the pull request to get stuck at the bottom of the
test queue and for the delay in merging just to stretch.
So, I'd propose that if possible the review process include a way
for reviewers to explicitly indicate, "This pull request is
provisionally approved subject to testing."
Approved pull requests would go on a separate priority test queue
with "first in, first out" policy. If the test suite passes,
they're auto-merged, if it fails they are removed from the queue
and must be re-approved.
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.
Does this sound workable/useful?
