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

Daniel Murphy yebblies at nospamgmail.com
Thu May 9 15:28:18 PDT 2013


"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.

>    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.  This 
often requires trying each original test case along with other measure to 
ensure the bug was actually fixed.  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.

> 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

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

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

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

A reviewer with some spare time then has a short list of 'ready to go' pull 
requests.  This would help a lot with dmd, I don't know about the other 
projects.

A useful third state would be 'Waiting for official review' which means the 
formal review process for phobos modules and Walter/Andrei approval for dmd 
enhancements.

I assume something this simple can be done with github issues...? 




More information about the Digitalmars-d mailing list