[Twisted-Python] overview: new review queue venue

[Clayton Daley]

To qualify my comments, I've yet to contribute to Twisted because I don't
have a good enough grasp of its internals, but I have contributed to a
variety of Git-based OSS projects.  I definitely get uneasy with the
general idea that we're trying to "replicate workflow A from Trac in
tangentially related Git PR feature".

We're in Git. We're hoping to solicit PRs from Git users. Git users will be
used to the way PRs are used in other OSS Git projects. Glyph has some
valid criticisms of the situation in some projects, but it should still be
the starting point. For example, closing a PR strikes me as a bad idea --
for lack of a better word, it feels "hostile" to me and certainly
unwelcoming.

In several of the projects I've seen, Git tags fill these roles. Piwik has
a "needs review" tag -- the short list for reviewers. Looks like it's a
manual add, but maybe it could be automated. Once reviewed, Piwik has tags
like "Tests & QA". ZendFramework has a generic "awaiting author update".

To address Glyph's concerns about lingering PRs, perhaps the combination of:

   - A policy like "a reviewer must accept, close, or tag with one of the
   next step tags"
   - A short list of common next steps like "code quality", "needs tests",
   "second opinion", "not review ready"... plus a generic "other author action"
   - Auto-close tickets except those with "needs review" or "second
   opinion" (say) 30 days after the last action.

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.


While an Issue is a good place for discussion about a problem, it lacks the
reference code often included in a PR.  You can't ask "how about this
approach" without showing the approach.  As an added bonus, most systems
run travis on PRs so you get a sense of "this approach is thorough" or
"this idea still breaks something".

Clayton Daley


On Sat, May 21, 2016 at 8:29 PM, Glyph <glyph at twistedmatrix.com> wrote:

>
> 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> 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
>
> _______________________________________________
> Twisted-Python mailing list
> Twisted-Python at twistedmatrix.com
> http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20160521/d535889c/attachment-0002.html>