D pullrequest review process rant

[Steven Schveighoffer]

On Wed, 08 May 2013 10:56:28 -0400, Benjamin Thaut  
<code at benjamin-thaut.de> wrote:

> I recently got very disappointed by the review process of a pull request  
> I did on druntime:  
> https://github.com/D-Programming-Language/druntime/pull/370
>
> This is how it went:
>
> 1) First everyone nitpicked about code formatting
> 2) I fixed all the nitpicks which was quite some work.
> 3) Pull request stalls for a month.
> 4) Even more nitpicks.
> 5) Even more work.
> 6) Reviewers actually start thinking about the problem behind the pull  
> request.
> 7) Problem is not important enough, review request gets rejected.
> 8) All the work, including fixing nitpicks is wasted.
>
> So what I'm trying to say is, that maybe a pull request should first be  
> analyzed if it is actually worth putting more work into it before  
> starting with the nitpicks. I don't know if the review process is  
> already defined somewhere, if not it might be worth doing so.
>

I first of all want to say, good for you to try helping the D community.   
This is something we need a lot of.  I also want to say that I am a  
contributor to both Phobos and Druntime, and I also have been largely  
unable to look at pull requests, shame on me.  This is super-frustrating  
for authors, and it's going to kill contributions if we don't fix it.

But I want to give a little bit more of an explanation here.  I was not  
involved in the pull request.  But I can see the history.  Some  
observations I have:

1. nitpicking of style is EASY, it requires no thought or real time,  
except to type the nit.  This makes it something more likely to occur  
before people have put any real thought into a pull request.  The lesson  
here should be to use the correct style so you don't have this problem.   
I'm aware that we don't have a good definition, or good consistency, and  
we need to do both of those.
2. The people who complained about style/formatting were NOT the same  
people who questioned the utility of the pull request.  This is HUGELY  
important in understanding how this all went down.

For point 2, realize that people have their own schedules and own day-time  
jobs.  I literally ignored the newsgroups for 6 months because I did not  
have time to look at D at all.  From the point of view of the requester,  
it is really important to understand that there is no real order to pull  
requests to review.  It's not like people can't review more than one pull  
request, have to review them in a specific order, or have to review them  
at all.  And the goals and motivations for review can be different.  We  
don't all speak as one voice, so try to remember that.

This is something I think can be better managed with a collaboration tool  
(like trello), or at least a status notification of who is assigned, who  
is reviewing, what status is it in, etc.  There is a large nebulous thing  
called a review process that is right now defined loosely (AFAIK, the only  
requirement is we have 2 people review each request), and pretty much  
differs for every person.  This needs to change.  We need an official  
review process, and a well-defined order of how things should be done.   
Otherwise, the requester has no clue what is happening, and even other  
reviewers have no idea what is happening.  I will start a new thread on  
this.

Sorry to read about this experience.  I hope we can all do better.

-Steve