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

Steven Schveighoffer schveiguy at yahoo.com
Thu May 9 18:25:16 PDT 2013


On Thu, 09 May 2013 18:28:18 -0400, Daniel Murphy  
<yebblies at nospamgmail.com> wrote:

> "Steven Schveighoffer" <schveiguy at yahoo.com> wrote in message
> news:op.wwt44fvkeav7ka at stevens-macbook-pro.local...
>> 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 suggest making the process as easy as possible for reviewers.  If it is
> too difficult, people will avoid doing it.  I speak from experience as  
> both
> a reviewer and reviewee.

I agree, and some of the stages I laid out can be notational, not official  
statuses.

There are two things I want to accomplish here:

1. Make the process defined and transparent so both reviewers and  
submitters understand what is going on, and what is expected.
2. Make someone own the review.  Without ownership, there will be long  
delays, and that reflects a sense of disapproval or apathy towards the  
submitter or his idea, even if that isn't the case.  I like how the  
"feature" review on the newsgroup has a review manager and he sets a  
deadline.  That works out REALLY well.

The recent thread on the review non-process outlined a story from the  
perspective of the submitter that looked to me very different than what  
actually happened from the reviewer side (reading the comments after the  
fact).

>
>>    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?
>
> This is what we are currently doing (sort of) and I think it would be  
> better
> to have the requester be responsible for closing the original bug.

I think we should push for full automation.  I think it can be done with  
github (and actually, I think it already automatically records the commit  
in bugzilla).

The problem I see with making the submitter do it is, the submitter may  
not be active at the time, or may not care.  The pull request is done, and  
he has his fix, but we need to make sure the bug list is updated properly.

> This
> often requires trying each original test case along with other measure to
> ensure the bug was actually fixed.

Oh!  I completely forgot!  Another requirement for a pull request to be  
accepted:

   g) Any fixed bugs MUST be accompanied by a unit test that fails with the  
current code but passes with the change.

If this is included, the automated tests should cover that.

> Sometimes pull requests are only partial
> fixes and while the requester (hopefully) knows this, the reviewer will  
> have
> to work all this out in addition to looking at the actual changes.

These would be more complex cases.  I think they would be rare (at least  
 from a library side, not sure about dmd).  It's important to note that  
reviewers are volunteers, and if you are not willing to take the time to  
do a full review, you should give it to someone else.

Perhaps the "description" requirement can be detailed to say "if this is  
only a partial fix, that should be noted."

> Ideally this goes on bugzilla or github issues so we don't end up
> fragmenting the information even further.

agreed.  I'm really leaning toward github issues since it already  
integrates with github pull requests (right? never used it before),  
probably involves less work to automate.

> My main problem is when I find I have time and energy to review some pull
> requests, I never know which ones are ready.  I usually find myself  
> looking
> through them from the front or the back, and rarely get very far.
>
> A big improvement would be a two-state system:
> - Ready for review
> - Not ready for review

I think this is important.  A pull request shouldn't be "Ready" until it's  
also submitted to the tracking system.  The issue should reflect the  
current state.  In other words, you should be searching for unassigned  
issues in the correct states, not pull requests.

> The submitter sets it to ready when they think it's done, and it goes to  
> Not
> Ready when any of the following happens:
> - An official reviewer decides it needs more work
> - It fails the autotester (ideally except for those intermittent  
> failures)
> - The submitter decides it needs more work

I think we are on the same page, my stages don't have to be "real"  
statuses, but it's important to formalize workflow and statuses so both  
sides know what is going on and what is expected.

-Steve


More information about the Digitalmars-d mailing list