[Twisted-Python] github, again

Jamu Kakar jkakar at kakar.ca
Mon Jun 3 18:35:54 MDT 2013


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.

Thanks,
J.



More information about the Twisted-Python mailing list