[Python-Dev] We should be using a tool for code reviews

[Brett Cannon]

On Wed, Sep 29, 2010 at 12:03, Guido van Rossum <guido at python.org> wrote:
> On Wed, Sep 29, 2010 at 11:47 AM, Brett Cannon <brett at python.org> wrote:
>> On Wed, Sep 29, 2010 at 11:41, Antoine Pitrou <solipsis at pitrou.net> wrote:
>>> On Wed, 29 Sep 2010 11:32:19 -0700
>>> Guido van Rossum <guido at python.org> wrote:
>>>> I would like to recommend that the Python core developers start using
>>>> a code review tool such as Rietveld or Reviewboard. I don't really
>>>> care which tool we use (I'm sure there are plenty of pros and cons to
>>>> each) but I do think we should get out of the stone age and start
>>>> using a tool for the majority of our code reviews.
>>>
>>> He, several of us would like it too (although for short patches it
>>> doesn't really make a difference), but what's missing is some kind of
>>> Roundup integration. Something as trivial as a "start review" button in
>>> front of every uploaded patch file would do the trick; it has been
>>> suggested several times already, but what's needed is someone to write
>>> the code :)
>>
>> The other option (as discussed on Buzz) is to add Rietveld's upload.py
>> to Misc/
>
> A problem with that is that we regularly make matching improvements to
> upload.py and the server-side code it talks to. While we tend to be
> conservative in these changes (because we don't control what version
> of upload.py people use) it would be a pain to maintain backwards
> compatibility with a version that was distributed in Misc/ two years
> ago -- that's kind of outside our horizon.

Well, I would assume people are working from a checkout. Patches from
an outdated checkout simply would fail and that's fine by me.

>
> Maybe the upload.py script distributed could just download the most
> current version from codereview.appspot.com/static/upload.py -- that
> URL is easy enough to keep stable.

That's fine by me.

>
>> and tell people to use that to submit the patch. Then we
>> simply say to the person submitting the patch, "upload it to Rietveld
>> and paste in the link" or simply require it upfront to encourage
>> people to do the upload in the first place. This would let usage to
>> move forward until we get that "start review" button (wasn't Ezio
>> looking into it?).
>
> Yeah, but it would still not work if they are working in an unpacked
> tarball -- upload.py requires that you have a VCS checkout of some
> sort (though it supports SVN, Hg, Git and Bzr).

How often do we even get patches generated from a downloaded copy of
Python? Is it enough to need to worry about this?