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/[email protected] ). 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
