[Twisted-Python] github, again

Glyph glyph at twistedmatrix.com
Mon Jun 3 19:00:42 MDT 2013


On Jun 3, 2013, at 5:35 PM, Jamu Kakar <jkakar at kakar.ca> wrote:

> Hi,
> 
> On Mon, Jun 3, 2013 at 4:48 PM, Christopher Armstrong
> <radix at twistedmatrix.com> wrote:
>> On Mon, Jun 3, 2013 at 3:59 PM, Glyph <glyph at twistedmatrix.com> wrote:
>>> One suggestion that almost everybody made immediately was: we should use
>>> Github for code reviews.
>> 
>> I'm +1 on the whole proposition as described.
> 
> Me too.
> 
>>> Finally, my own minor concern: Github has no notion of a "code review" as
>>> a unit of work.  A pull request is just "open" until it is "closed".
>>> Closing pull requests to request changes would be jarring to the cultural
>>> norms associated with Github's UI.  All the github users I've spoken with,
>>> even those who follow processes which are effectively identical to
>>> Twisted's, have assured me that this is not really an issue.  A code review
>>> is "accepted" when you merge it; it's "rejected" if the pull request is
>>> still open but has some comments on it.  This will make porting over
>>> <http://twistedmatrix.com/highscores/> a bit challenging, but I think it
>>> would be worth letting that break for the time being.
>> 
>> I don't really like the idea of maintaining review state in Trac, especially
>> since one of the points of this discussion is to make the life of the
>> reviewer easier. My feeling is that the slight difference in review workflow
>> on PRs -- the fact that there is no "responsibility transfer" mechanism --
>> will not be a serious problem in practice, and that we should have a culture
>> of closing abandoned PRs.
> 
> Something that has worked well for me on other projects is to use
> simple conventions.  When you finally approve a branch you leave a
> comment like 'jkakar:approve'.  If you expect changes you leave a
> comment like 'jkakar:needs-fixing'.  In other words, you don't really
> need an app-enforced mechanism if you have a simple convention.  I
> propose starting with the simplest convention: the reviewing must add
> an 'author:approve' comment when they're finally happy.

Honestly, we don't have much of an enforcement mechanism as it is.  We just manually add the 'review' keyword when something goes into review, by typing 'review' into the keywords box; you also have to manually un-assign the ticket, and people often forget that part.

The two things that notice this are our IRC bot and our High Scores page.

The lack of a 'keyword' mechanism somewhat distracted me from the main issue though; if we just had high scores and the announcement bot scan the comments for the strings "Please fix." for out-of-review and "Please review." for back-into-review, we could both maintain the exact same workflow and reward politeness ;-).

Thanks for the suggestion, Jamu!

-glyph




More information about the Twisted-Python mailing list