[Twisted-Python] Defining the review workflow on top of GitHub PR

[Glyph]

> On Sep 30, 2017, at 3:14 PM, Adi Roiban <adi at roiban.ro> wrote:
> 
> I am restarting this discussion
> https://twistedmatrix.com/pipermail/twisted-python/2016-May/030333.html
> 
> I am starting a new thread since I want to keep the focus on the
> review process / workflow / markers, and not on the things required to
> accept a PR or do a review.

I'm a little confused.  What do you mean by "process / workflow / markers" if not "the things required to accept a PR or do a review"?

> ----------
> 
>> Proposing: Just open a pull request.  Any open pull request should be treated as part of the queue.
> 
> I don't like this. If you are not a comitter, you need to open a PR to
> trigger the tests.
> 
> So you want to first open a PR, then wait for tests to execute, then
> fix and only after that to request the review.
> 
> We can start with setting the title to have "[WIP]" marker, to let
> others know that this is not yet ready... but then when changes are
> required, the reviewer will have to set the WIP marker again.. and if
> the reviewer is not a team member, it will not have rights to edit the
> subject.
> 
> But I hope that we can have a bot which once a "please review" comment
> is left, it will set a label.

Mark Williams already started working on this: https://github.com/markrwilliams/txghbot

>> Accepting: A committer pushes the big green button;
> 
> +1 ... but maybe also leave a comment :)

We should have more verbiage around the comment stuff, including the "always say thank you twice" rule which I don't think is written down anywhere :-).

>> Reviewing: This is the potentially slightly odd part.  I believe a review that doesn't result in acceptance should close the PR.  We need to be careful to always include some text that explains that closing a PR does not mean that the change is rejected, just that the submitter must re-submit.  Initially this would just mean opening a new PR, but Mark Williams is working on a bot to re-open pull requests when a submitter posts a "please review" comment: https://github.com/markrwilliams/txghbot
> 
> Since we will have a bot for "please review", why not use the same bot
> to set a label on "please make changes" ?

Yep!

> I think that closing a PR should mean that the work on that branch is
> rejected :)

I still want to require people to open an issue first so we can separate closing PRs ("this patch is too bad to be accepted, please try again") from closing tickets or issues ("we do not want to do this work").

>> Responding: A submitter can open a new PR, or, once we start running txghbot, reopen their closed PR.
> 
> As commented above, I am +1 for leaving a "please review" comment and
> having a bot updating the labels.
> 
>> Viewing: https://github.com/twisted/twisted/pulls?utf8=✓&q=is%3Apr+is%3Aopen+-status%3Afailed
> 
> One we get the "please-review" and "changes-needed" labels it should
> be eaiser to view the queue.

I can update https://twisted.reviews/ <https://twisted.reviews/> (at some point) when to point at the appropriate link.  (You've all got it bookmarked, right?)

> -------
> 
> Whem multiple reviewers are required, you can use the dedicated GitHub
> Review message and approve it without hiting the merge button.
> 
> ---------
> 
> I have no idea how other projects are managing the review queue.

"Poorly" :-).

A failure mode of many open source projects is that they have terrible latency when responding to new contributors.

> Please send your feedback.
> 
> If we agree on a process based on managing the labes, I can work on
> implemeting the required logic with a bot and GitHub hooks.

Please do coordinate with Mark on this, if you weren't already ;-).

> --------
> 
> We can also start by using the WIP marker
> 
> * while preparing the PR
> * once changes are required and the author works on addressing the
> changes requsted on review
> 
> Any PR which is open and does not have the WIP marker means that is
> part of the queue.
> 
> ----------
> 
> 
> Thanks!
> 
> PS: I have checked pyca/crypography but I don't see any pattern there
> and a lot of PR are merged without any comment
> https://github.com/pyca/cryptography/pulls?q=is%3Apr+is%3Aclosed <https://github.com/pyca/cryptography/pulls?q=is:pr+is:closed>

Cryptography is kind of a weird project with a somewhat rarified contributor audience, I don't know if it makes sense to copy their process.

-g
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20171001/bfcf0bd9/attachment.html>