Hi, On Mon, Jun 3, 2013 at 4:48 PM, Christopher Armstrong <ra...@twistedmatrix.com> wrote: > On Mon, Jun 3, 2013 at 3:59 PM, Glyph <gl...@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. _______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
