[Twisted-Python] overview: new review queue venue

[Glyph]

> On May 21, 2016, at 10:52 PM, Craig Rodrigues <rodrigc at crodrigues.org> wrote:
> 
> On Sat, May 21, 2016 at 3:12 PM, Glyph <glyph at twistedmatrix.com <mailto:glyph at twistedmatrix.com>> wrote:
> Hooray!  We're on github now.  Next: there's the question of how to deal with pull requests?
> 
> A few people, including myself modified the text with Git instructions:
>  https://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch <https://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch>
> 
> The basic idea is, "for now, if you submit a GitHub PR, you must file a Trac ticket.  You must reference the PR in the Trac ticket,
> and you must reference the Trac ticket in the PR".
> If you want to change that workflow somehow, go ahead, but for starters that should be good enough to get going.
> 
> These two pull requests followed that:
> 
> https://github.com/twisted/twisted/pull/62 <https://github.com/twisted/twisted/pull/62>
> https://github.com/twisted/twisted/pull/63 <https://github.com/twisted/twisted/pull/63>

Sorry, I guess I wasn't clear.  I know that PRs are presently a potential alternative to a diff, and that we are still using Trac for ticketing.  I want to make it possible to avoid using Trac for ticketing; perhaps switching to github issues entirely.  Right now, PRs are still ignored; the thing reviewers are paying attention to is the review queue in Trac, and that is sub-optimal, since it requires new contributors to do something unfamiliar.

> 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 <https://github.com/markrwilliams/txghbot>
> 
> I don't agree with this.  If a PR is reviewed, and the result of the review is "NO WAY", then I am OK with the PR being closed.

I understand that this is your feeling, but do you have any reasoning as to why you believe that this should be the case?  "I don't feel like it" isn't selling me.

> However, if a the result of the review is "needs more work before being accepted", then
> it should be possible for the submitter to add more commits to that PR,

Technically speaking, "PRs" are not things that you add commits to.  You add commits to branches, and the PR points to a branch.  Closing your PR will not - can not, if it's on your own fork - delete your branch.

> and even "git rebase" and squash commits in that PR.

Please do not use squash commits.  See http://mjg59.dreamwidth.org/42759.html.

> That's the process that I followed when I submitted https://github.com/twisted/twisted/pull/62 <https://github.com/twisted/twisted/pull/62>
> 
> By the way, PR 62 is the first pull request that has been successfully submitted to Twisted, and incorporated into the code:
> 
> https://github.com/twisted/twisted/commit/95c49d74136eef420fabdeea85f650da2bc78b07 <https://github.com/twisted/twisted/commit/95c49d74136eef420fabdeea85f650da2bc78b07>
> 
> It is a rather trivial documentation fix, but I will still take credit as the first Pull Request successful submitter! :)

First is first, trivial or not :-).

-glyph


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