D pull request review process -- strawman formal definition, query for tools

[Steven Schveighoffer]

The review process for D pull requests is basically non-existent.  We need  
to change this.  Without any understanding of how the process works, both  
reviewers and pull requesters are left to their own devices and  
assumptions to determine what is going on (see recent anecdote on a pull  
request: http://forum.dlang.org/post/kmdp2n$at4$1@digitalmars.com ).  We  
currently have a nice (informal but defined) process for large library  
changes, and it seems to work well.  We should do the same at a smaller  
scale, for pull requests.

I want to define a process for a requester to follow in order to submit a  
pull request, including pre-review requirements, objective measurements  
for acceptance, and review workflow/schedule.  We also need a way to have  
people generally responsible for reviews.  At the moment, reviews are  
ad-hoc, with no clear schedule or assignment.  This can result in  
misconceptions and bit-rotting of pull requests.  We need to try and  
present a friendly and inviting process so that people actually WANT to  
contribute (see previously mentioned thread).  I realize this is a large  
commitment, but if we don't have some sort of responsibility (myself  
included), we won't do it.  There are a lot of unpulled pull requests, and  
I think this can be a very detrimental/demotivating thing for a  
community-driven project.

To that end, here is a strawman for discussion.  This is for druntime and  
phobos only, I don't know if this will work for dmd or other projects:

1) pull request prerequisites
    a) pull must pass auto tester before being reviewed.  If you submit a  
pull request and it fails, you need to fix it before a review can be  
done.  Exceptions can be made if the auto tester isn't able to test the  
code properly (e.g. you have a pull for phobos and a pull for druntime  
that depend on each other).
    b) pull request description is sufficient to understand the reasoning  
behind the request.
    c) any related bugzilla requests should be identified.  If this fixes a  
bug, the bug MUST be in bugzilla before pull request is reviewed.  It is a  
good idea to make sure this is actually a bug before creating a pull  
request!  Use the forums/newsgroups.
    d) If pull request is for a large feature (e.g. replacement or addition  
of a module), the library change voting process should be followed on the  
newsgroups instead of this process.

2) pull request review process stages
    a) submitted - pull request awaiting initial verification of pull  
request prerequisites.
    b) unassigned - pull request passes prerequisites, and is awaiting  
assignment to an approved reviewer (process for assignment TBD)
    c) ready for review - pull request is assigned, but reviewer has not  
started looking at it.
    d) official review - pull request in review by reviewer.  Next stage  
can be e - h.
    e) needs work - pull request needs work to be able to be accepted (only  
use this stage if original requester is not immediately responsive).  Go  
back to c after fixed.
    f) rejected - pull request is rejected.  Can be re-opened by another  
official contributor.
    g) approved - pull request is approved for pull.
    h) conditionally approved - pull request is approved, but with minor  
changes (e.g. comment or ddoc changes).
    i) pulled/closed - pull request is approved by second reviewer (this  
does not need to be an in-depth review)

3) Objective measurements required for pull.  All pulls must pass these  
requirements, plan accordingly.
    a) Passes auto-tester on all platforms.  Exceptions can be made if the  
auto-tester fails because of a limitation in the auto-tester, but must  
pass unit tests on at least one failed platform if that is the case.
    b) Code is formatted properly (TBD).
    c) 2 reviewers must approve of the feature.  Secondary review does not  
need to be in-depth
    d) License must be boost.  Any copyrights must be CORRECTLY attributed  
to authors in the designated comment area (if you used code from somewhere  
else, you must include the appropriate copyright information, and the  
license must be able to be re-licensed as boost).  Any large port of code  
should include a courtesy link to the original project unless the original  
project's author is OK with not including the link.
    e) Any related bugzilla entries must be referenced BY LINK in the pull  
request, and the bugzilla entries must reference BY LINK the pull  
request.  This is the requester's responsibility to set up.
    f) Any public-facing code must be properly documented for DDoc to  
generate.  At this point, the actual docs don't have to be verified as  
properly building (because it's freaking annoying to build them).  If the  
auto-tester is able to do this at some point, then this would become a  
requirement.

4) Reviewers' roles and responsibilities.  I am not defining how a  
reviewer gets assigned, not sure how that should work, and it likely  
depends on the tool we use.
    a) When reviewer accepts role as primary reviewer, he/she should review  
the pull request or decline the role as soon as possible.
    b) if the role is accepted, the reviewer should review the pull request  
within a reasonable time frame (what is this, a month?  a week?), or at  
least acknowledge how long they think it should take to get to it.   
Feedback is very important so contributors are not discouraged.
    c) once the review has started, for complex pull requests (or issues  
with the reviewer's schedule) that would require more than a few days or  
so to review, the reviewer should note this.
    d) If there are extra volunteer participants during the review, the  
reviewer should moderate the discussion and comment on review criticisms  
that are not clear-cut.  In other words, the reviewer should be the  
official word on whether a criticism is valid and requires a change.  Not  
sure if there should be some sort of appeals system.
    e) Primary reviewer is responsible for ensuring any objective  
measurements are fulfilled that AREN'T automatically handled (we should  
handle as much as possible automatically).
    f) Secondary reviewer does not need to do an in-depth review.  This  
should not take more than 5-10 minutes.
    g) The primary reviewer is responsible for changing pull-request status  
in whatever tool we choose to track this.
    h) One of the reviewers (not sure if it should be primary or secondary)  
should close any bugzilla bugs fixed by the pull request.  Can this be  
automated?

==================

OK, so now to implement this kind of thing, we need to have a  
collaboration tool that allows tracking the review through its workflow.   
No idea what the best tool or what a good tool would be.  We need  
something kind of like an issue tracker (can github issue tracker do  
this?).  I don't have a lot of experience with doing online project  
collaboration except with D.  I like trello, but it seems to have not  
caught on here.

-Steve