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?

Reply via email to