Managing Pull Requests...

Dec 28, 2011 at 7:49 PM

We should come up with a system for handling pull requests so we're consistent. I prefer not to leave them lying around indefinitely. So here's what I propose.

  1. A pull request is neither accepted nor declined until at least one core member has reviewed it. We should strive to at least start a review quickly.
  2. A pull request should always be associated with an issue and ideally have an associated discussion so we're all on board with the proposed change.
  3. After a review, if the review comments haven't been addressed by the person submitting the pull request, we decline the pull request. Note, the person who issued the pull request can always re-submit the pull request later when they've addressed the issues. This just keeps the pull request list manageable.
  4. We should always strive to encourage folks to fix issues in their pull requests. We're trying to groom more contributors. However, if someone doesn't respond to review comments, but the changes are things we want, a core contributor can choose to make the changes themselves. Probably should issue a new pull request and close the old one with the improved changes.
  5. We should only "accept" the pull request after we've merged it in.

Does this make sense? Anything you'd change?

Dec 28, 2011 at 7:54 PM

I agree that it should be reviewed by at least one core member but I think there should be a second set of eyes before the final merge/acceptance. Just that extra check to catch anything the original review might have missed. The second review should come at the end, just once, after any changes/fixes have been submitted. Not saying the contribution should be restricted to one reviewer only but minimum one for initial review and minimum two for acceptance.

Dec 28, 2011 at 8:03 PM
Edited Dec 28, 2011 at 8:03 PM

So Phil, how is it different from how we're doing today?

Dec 28, 2011 at 8:29 PM

Sounds like a good process.

Dec 28, 2011 at 8:42 PM

@dotnetjunky it really isn't all that different than what we do today, other than I noticed some pull requests that haven't been commented on for over a week. I'm just trying to codify what we're doing so it's clear to everybody when we decide to decline a pull request.

Perhaps the other note we can add is the core team can decline a pull request immediately if we don't think the feature makes sense for the product. For example, if a random pull request with no issue or discussion comes in and we don' t think it's a good fit, we can decline immediately.

Dec 28, 2011 at 9:01 PM

Hmm, anyone know how to re-open a declined pull request? I'm thinking this workflow might not work well. I declined a pull request and the person addressed the comments, and it did not re-open it.

Dec 28, 2011 at 9:02 PM

OK, ahem, for the record I only just read this.

I think allowing contributors to respond to comments before hitting the reject button, especially on larger pull requests, is far less discouraging that writing a bunch of comments and hitting decline - it would certainly reduce my unhealthy alcohol consumption.

As a new contributor, who would have loved encouraging, I've found the experience extremely discouraging - to the point of writing long, alcohol induced, and depressing rejection letters that have taken me right back to high school.

Dec 28, 2011 at 9:24 PM

Yeah, I don't think we should decline pull requests until we're fully ready to decline it. It's not an appropriate way to "ping" the person sending a pull request because there's no way to re-open it. I thought it would work like CodePlex issues do. With an issue, you can close it, but the person who opened it originally can re-open it if they feel it was closed in error.

Apparently that's not the case with a pull request.

Instead, we can use direct messages (click on their username and send them a message via CodePlex). 

Dec 28, 2011 at 9:30 PM

As a pull request author I get an email everytime someone comments anyway - and I can't see an option to switch off, so I don't think DM is necessary, instead commenting directly on the pull request is fine so long as it's not closed.

Dec 28, 2011 at 9:32 PM

That's good to know. I've seen a lot of pull requests that hadn't received any responses from the author so I was wondering if the review comments were not making it to them.

Dec 28, 2011 at 9:35 PM

Ooops I'm talking crap, I only got the decline notice, not the comments.

Dec 28, 2011 at 10:17 PM

To be clear, we can use DM to ping folks that there is something to look at, but we should not have actual discussion about the issues on there. Otherwise, no one else can see it, which is obviously bad. I'm guessing that's what you meant, but just making sure :)

Dec 28, 2011 at 10:30 PM

FYI, I started a somewhat related discussion about keeping a working spec for complex changes. It's all part of improving our pull request workflow, which clearly has room for improvement.

Dec 29, 2011 at 12:55 AM

Yes, that's what I meant. DM is just a notification. Discussion should happen here and ideally summarized in a working spec.