[Twisted-Python] overview: new review queue venue

[Glyph]

> On May 21, 2016, at 3:57 PM, Terry Jones <terry at jon.es> wrote:
> 
> Hi Glyph
> 
> On Sun, May 22, 2016 at 12:12 AM, Glyph <glyph at twistedmatrix.com <mailto:glyph at twistedmatrix.com>> wrote:
> Reviewing: This is the potentially slightly odd part.  I believe a review that doesn't result in acceptance should close the PR.
> 
> This feels wrong to me. I find github pull requests very useful, in ways that it sounds like your suggestion would cut off - if I understand you right. Do you mean that the one person who decides to do the formal review would set themself as the "assignee" and, following their review, close the ticket if the change isn't up to standard?

The "assignee" field isn't all that useful; really, it should be set to the submitter, but github has weird rules about who can be assigned (last I checked, only contributors).

> And in the meantime others (who are not the assignee) would be free to comment at will, with the pr staying open?

Drive-by comments on a PR are sometimes helpful, but should be used sparingly.  Mostly, discussion should happen on issues, not PRs.  A PR is a suggested resolution to a problem, and we might reject one solution, but an issue should describe the problem itself.

> If so, that's good, but I still don't like to think that all discussion around a pr then gets shut down on the say so of the single reviewer.

The PR gets closed, not deleted.  People can still comment if they like.

> One thing that pull requests encourage is discussion - sometimes it's a way to ask for input on how to proceed, sometimes it's just people chipping in a little bit with a code optimization, sometimes people saying "if you do this then this other thing will happen" etc. They help people learn, to easily make small contributions (that can lead to larger ones), to see what's going on, to judge the health of a project, etc. I like how dynamic and lightweight that process can be in a Github pull request. It feels to me that closing pull requests is a step in the other direction - back towards something that feels more monolithic and more old fashioned.

You see "dynamic and lightweight", I see "unfocused and noisy" :).

One of the things I still don't like about github is the tendency for projects to build up huge piles of open PRs, which nobody wants to reject because they're maybe possibly useful, but they won't accept because they don't meet quality standards.  Contributors don't get clear feedback about whether they're expected to do more or whether the project will take it over, and people frequently get mad about their stuff not being merged.  I think by closing PRs that we aren't going to merge as-is we can send a much clearer signal about what the project is taking on versus what it expects external contributors to do more work on.

> As usual, I'm sure that there's absolutely nothing I can say on any tech subject that you don't know already :-)  But those are my thoughts...


While I disagree, I also think that this is a very common perception, and another one of the functions of the github bot could perform would be a form comment after the PR is closed, to always submit a form comment to explain what closing the PR means and how to reopen it.

-glyph
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20160521/59159b41/attachment-0002.html>