On Wed, 29 Sep 2010 11:32:19 -0700 Guido van Rossum 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 :) Regards Antoine.

On Wed, Sep 29, 2010 at 11:41 AM, Antoine Pitrou wrote:

On Wed, 29 Sep 2010 11:32:19 -0700 Guido van Rossum 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 :)

As I tried to explain in the Buzz thread (not that I require you to read it -- I'll happily repeat myself here): Unfortunately taking the average patch posted to the tracker and importing it in Rietveld is very iffy -- it's very hard to find the right branch+rev needed to be able to apply the patch correctly -- not to mention that there are so many (slightly) different patch formats. It would take a fair bit of AI to get this right. The recommended way to work with Rietveld is to use a checkout (fortunately with Hg this will become easier to do) and use the "upload.py" script that you can download from Rietveld (log in and click on the Create link). Other projects that have adopted Rietveld (Chromum, GWT, Go) require its use for all reviews and have written their own scripts extending upload.py to implement the workflow they like (each one a bit different). They've also created their own branded Rietveld sites (which is easy to do using App Engine). -- --Guido van Rossum (python.org/~guido)

On Wed, Sep 29, 2010 at 11:47 AM, Brett Cannon wrote:

On Wed, Sep 29, 2010 at 11:41, Antoine Pitrou wrote:

...

On Wed, 29 Sep 2010 11:32:19 -0700 Guido van Rossum 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. 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.

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). -- --Guido van Rossum (python.org/~guido)

On Sep 29, 2010, at 11:32 AM, Guido van Rossum 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.

I like and have used Rietveld, both as a submitter of a patch and as a reviewer of someone else's code. It's very nice, and I think we should use it where appropriate, I don't think it should be a requirement. While it will be somewhat better integrated into our normal development processes whenever we move to Mercurial, it won't be seamless. I don't particularly like having to run a separate script (upload.py IIRC) in order to initiate a review and push updates. One thing I really like about Launchpad's merge proposals is that it's very well integrated into normal workflows. Updates against the target branch are automatically tracked in the generated diff and in fact, once a merge proposal has been accepted, it can be automatically landed by a 'bot if you want. Launchpad's merge proposal system doesn't have the really nice web-based ui that Rietveld has, but it is well integrated with an email-based workflow. When I see a merge proposal come into my inbox, with the diff against the target branch, I can just reply with my review inline right there, and those comments are visible to all subscribers. It lowers the barrier to performing the review immensely. Web is nice and should be available, but I really do not want to give up on email-based reviews (well, with Python give up on the possibility of them). Someone else mentioned that it should better integrate with Roundup, and I agree with that. I would much rather we concentrate on converting over to Mercurial as soon as possible, since I think a dvcs will do more to improve our processes than anything else at this point. Please, please, please, let's not let it wait until Pycon 2011 (*2 years* after pronouncement) [1]. -Barry [1] Apologies for sounding critical of any individual - that's not my intent. Dirkjan and folks have done a lot of great work to this point and ISTM that we're *really* close. Let's JFDI and work out remaining kinks as we go!

On Wed, Sep 29, 2010 at 12:03, Guido van Rossum wrote:

On Wed, Sep 29, 2010 at 11:47 AM, Brett Cannon wrote:

...

On Wed, Sep 29, 2010 at 11:41, Antoine Pitrou wrote:

...

On Wed, 29 Sep 2010 11:32:19 -0700 Guido van Rossum 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?

On Wed, Sep 29, 2010 at 4:23 PM, Guido van Rossum wrote: ..

But maybe with Hg it's less of a burden to ask people to use a checkout.

I thought with Hg it would be more of a burden for casual contributors to use a checkout simply because the checkout is many times bigger.

On Wed, Sep 29, 2010 at 1:31 PM, Alexander Belopolsky wrote:

On Wed, Sep 29, 2010 at 4:23 PM, Guido van Rossum wrote: ..

...

But maybe with Hg it's less of a burden to ask people to use a checkout.

I thought with Hg it would be more of a burden for casual contributors to use a checkout simply because the checkout is many times bigger.

Isn't it still faster though? -- --Guido van Rossum (python.org/~guido)

Le mercredi 29 septembre 2010 à 13:35 -0700, Brett Cannon a écrit :

Well, we can start with strongly worded suggestions that patches submitted using Rietveld will typically get priority over patches submitted just to the issue tracker and that this means doing it from a checkout.

But will we (all of us) actually follow this rule? Granted, a patch is reviewed faster if it's easier to review. But in many cases (small patches) it doesn't really make a difference. I have from time to time suggested that a contributor post his/her patch to Rietveld. But that was for really large or nasty ones. Regards Antoine.

On Wed, Sep 29, 2010 at 1:43 PM, Antoine Pitrou wrote:

Le mercredi 29 septembre 2010 à 13:35 -0700, Brett Cannon a écrit :

...

Well, we can start with strongly worded suggestions that patches submitted using Rietveld will typically get priority over patches submitted just to the issue tracker and that this means doing it from a checkout.

But will we (all of us) actually follow this rule? Granted, a patch is reviewed faster if it's easier to review. But in many cases (small patches) it doesn't really make a difference.

I have from time to time suggested that a contributor post his/her patch to Rietveld. But that was for really large or nasty ones.

More use of the tool also increases accountability and provides more opportunities for junior developers to learn. (And it increases familiarity of all involved with the tool for the future.) I agree it shouldn't be mandatory, but I would suggest you give it a try even for small changes. -- --Guido van Rossum (python.org/~guido)

On Wed, Sep 29, 2010 at 13:43, Antoine Pitrou wrote:

Le mercredi 29 septembre 2010 à 13:35 -0700, Brett Cannon a écrit :

...

Well, we can start with strongly worded suggestions that patches submitted using Rietveld will typically get priority over patches submitted just to the issue tracker and that this means doing it from a checkout.

But will we (all of us) actually follow this rule?

Does it matter? If it leads to an easier review process in some cases and doesn't impact reviews overall in a negative way who cares.

Granted, a patch is reviewed faster if it's easier to review. But in many cases (small patches) it doesn't really make a difference.

So it's a no-op in some cases, a benefit in others. I don't see the harm.

I have from time to time suggested that a contributor post his/her patch to Rietveld. But that was for really large or nasty ones.

Same here, but that was because I didn't have an easy way to say "submit the patch to Rietveld and add the link to the issue". If we have it in a checkout we can easily say "please run code_review.py from your checkout and paste the link into the issue to move the review forward."

On Wed, 29 Sep 2010 13:56:46 -0700 Brett Cannon wrote:

On Wed, Sep 29, 2010 at 13:31, Alexander Belopolsky wrote:

...

On Wed, Sep 29, 2010 at 4:23 PM, Guido van Rossum wrote: ..

...

But maybe with Hg it's less of a burden to ask people to use a checkout.

I thought with Hg it would be more of a burden for casual contributors to use a checkout simply because the checkout is many times bigger.

Many times bigger than what? If you mean svn that's not true (the eval of the DVCS pegged Hg at only 50% larger than svn). If you mean compared to the tar.bz2 file, then sure, but a zip of an Hg checkout is under 50 MB which is less than 5x larger.

The disk footprint is not very much larger, but I assume that the amount of bandwidth needed for a first checkout can easily be 10x or 50x. (then, if you keep the checkout around, pulling new revisions is very efficient) That said, it seems there are tools to optimize the internal structures of an hg repository so that it takes less disk space (and therefore less checkout bandwidth). I don't know whether Dirkjan intends to use that. Regards Antoine.

One other thought: IME patches in general are suboptimal to branches, so I think we should be encouraging people to publish their branches publicly for review. A diff is a decent way to get feedback about code changes, but that's often only part of the work involved in deciding whether a change should be accepted or not. A reviewer often wants to do a build with the changes, test them on various tasks and application, run the test suite, etc. For this, "merge" is much better than patch(1). -Barry

On Thu, Sep 30, 2010 at 4:47 AM, Brett Cannon wrote:

On Wed, Sep 29, 2010 at 11:41, Antoine Pitrou wrote:

...

On Wed, 29 Sep 2010 11:32:19 -0700 Guido van Rossum 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.

Somewhat amusing to get to this thread a few minutes after creating a Reitveld issue for the first pass of my urllib.parse patch :)

...

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/ 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?).

Make our script a "submit_patch.py" wrapper around the vanilla "upload.py" rather than a replacement of it and it sounds like a good idea to me. I'd want to be able to do a signature check on upload.py before we enabled runtime downloading of new versions from the codereview site though (in the meantime, a vetted version checked into SVN would do the trick). Even with Roundup integration, being able to create/update the Reitveld issue and make the appropriate tracker updates with a single command would still be handy. Cheers, Nick. -- Nick Coghlan   |   ncoghlan@gmail.com   |   Brisbane, Australia

On Sep 29, 2010, at 05:22 PM, Alexander Belopolsky wrote:

...

Many times bigger than what? If you mean svn that's not true (the eval of the DVCS pegged Hg at only 50% larger than svn).

My experience was different. I may misremember because I did not try to use Hg since about a year ago, but the Hg checkout was 3-4x of that of an SVN branch. However, my comment was simply a reaction to the argument that Hg checkout would be *less* of a burden than SVN.

With Bazaar, if you're a frequent contributor to a project, you can amortize the expensive initial checkout across all the branches you'll ever make. The first branch may take a while, but subsequent ones are very fast. I'd be surprised if Mercurial didn't have at least something similar. It's true though that for drive-by contributors who rarely develop a patch, generating a branch may be too high a barrier. In that case, I think it's fine to have diffs (which a more experienced developer can turn into a branch).

...

But honestly, the line has to be drawn somewhere. Right now we won't take a patch submitted to the mailing list, but accepting patches not against a checkout wastes time for everyone involved as soon as it doesn't apply cleanly. At that point either a core developer has to fix it or (what typically happens) the person has to get a checkout, update their patch, and then re-submit. We might as well minimize that when possible by really pushing for checkout patches.

Well, many patches do not apply cleanly by the time they are reviewed even when they are originally produced from a clean checkout. I actually wonder if it would be appropriate to encourage *reviewers* to upload the patches to a review tool. A reviewer is much more likely to have a clean checkout already than a casual contributor and oftentimes applying the patch is the first thing reviewers do anyways. If a review tool is one command away, it may be even easier to run it rather than to figure out how to reference the code in the patch in the roundup comment.

While branches are far better than patches here, you can still have conflicts when merging the branch to the trunk (which I recommend doing when reviewing a branch). I have no problem halting a review right there and asking the developer to ensure a conflict-free merge. It's of course at the reviewer's discretion to assist them with this (e.g. by branching their branch, resolving the conflicts, committing and publishing this clean branch, for the original developer to merge into *their* branch before requesting a re-review -- see why branches are so much better?! :). -Barry

That shouldn't be too hard. Someone just has to create an App Engine project and handle the deployment. I guess the trickiest part is making sure enough people have admin access so multiple people can update the website, especially if we run a modified copy so that bugs.python.org can push "anonymous" (and probably be the only thing that can).

I was hoping that I can run Rietveld on the same machine as roundup, with the added advantage that people can use the same logins, and even the same cookies. Of course, we could do an OpenID-style indirect communication, referring people to Roundup when they need to be authenticated to Rietveld (in particular when commenting). Regards, Martin

...

So perhaps we should just run our own Rietveld instance next to Roundup.

Would it be possible to sync up the reitveld issue numbers with the roundup ones if you did that?

Most certainly. However, this works fairly well today already. If you put [issueNNNN] into the Rietveld subject, it can already synchronize with roundup (even though it would use different issue numbers). For a local copy of Rietveld, I could certainly arrange it to use the very same numbers.

Or would the fact that a single issue can have multiple attached patches prevent that?

No, Rietveld can deal with multiple patch sets fine (not sure how it fares when they are unrelated patches, since it also computes deltas between subsets - but apparently only for the same files). Regards, Martin

On Wed, Sep 29, 2010 at 14:58, "Martin v. Löwis" wrote:

...

That shouldn't be too hard. Someone just has to create an App Engine project and handle the deployment. I guess the trickiest part is making sure enough people have admin access so multiple people can update the website, especially if we run a modified copy so that bugs.python.org can push "anonymous" (and probably be the only thing that can).

I was hoping that I can run Rietveld on the same machine as roundup, with the added advantage that people can use the same logins, and even the same cookies.

If it can work that sure then great. I just don't know what it takes to run Rietveld sans App Engine. -Brett

Of course, we could do an OpenID-style indirect communication, referring people to Roundup when they need to be authenticated to Rietveld (in particular when commenting).

Regards, Martin

On Wed, Sep 29, 2010 at 5:57 PM, Nick Coghlan wrote:

Would it be possible to sync up the reitveld issue numbers with the roundup ones if you did that? Or would the fact that a single issue can have multiple attached patches prevent that?

Another quirk would be that often several pieces are uploaded which really constitute a single change; often docs and/or tests are provided separately, just because we've had to go back and ask for them. Sometimes they're provided by a separate contributor. Keeping the numbers aligned could probably be done based on the file number containing the patch, assuming those are never reused (I don't think they are, but haven't dug deep enough into roundup).   -Fred -- Fred L. Drake, Jr.    <fdrake at acm.org> "A storm broke loose in my mind."  --Albert Einstein

On Wed, Sep 29, 2010 at 3:12 PM, Antoine Pitrou wrote:

On Wed, 29 Sep 2010 23:58:05 +0200 "Martin v. Löwis" wrote:

...

...

That shouldn't be too hard. Someone just has to create an App Engine project and handle the deployment. I guess the trickiest part is making sure enough people have admin access so multiple people can update the website, especially if we run a modified copy so that bugs.python.org can push "anonymous" (and probably be the only thing that can).

I was hoping that I can run Rietveld on the same machine as roundup, with the added advantage that people can use the same logins, and even the same cookies.

That would be very nice. Aren't there some App Engine emulation layers? Or are they only for development?

Yes, http://code.google.com/p/django-gae2django/ (see also http://code.google.com/appengine/articles/pure_django.html) -- --Guido van Rossum (python.org/~guido)

On Wed, Sep 29, 2010 at 20:32, Guido van Rossum 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.

Rambling thoughts about some of the things mentioned in this thread. I think hg-review looks interesting, though it may not (yet) have the level of sophistication of Rietveld. (Public test instance at http://review.stevelosh.com/.) It might be interesting to integrate Rietveld uploads in a Mercurial extension, particularly if it gets integrated with mq somehow. Email reviews seem to actually work really well for Mercurial (using the patchbomb extension to send out patches to mailing lists); the only problem we run into is that we can't keep track of things that have been submitted and reviewed. But it makes commenting inline effortless and provides a familiar interface for everyone. For the imparting wisdom thing, I think that's more a culture thing than a tool thing. If reviews happen in public as a standard part of the process, then it probably won't appear judgmental or overbearing either in a tool or in email (or issue tracker). I hope people will discover and like mq, which makes it easy to keep together a coherent series of patches. Cheers, Dirkjan

Barry Warsaw writes:

You can have "co-located" branches[1] which essentially switch in-place, so if a branch is changing some .c files, you won't have to rebuild the whole world just to try out a patch.

In Mercurial these are called "named branches", and they are repo-local (by which I mean they must be part of the DAG). Named branches used to have some inconvenient aspects relevant to standalone branches (they could be fairly confusing to other users if pushed before being merge to mainline). It's not obvious to me that Mercurial style named branches would work well here ... it would take a little thought to design an appropriate workflow, anyway.

On Thu, Sep 30, 2010 at 07:45:52AM +1000, Nick Coghlan wrote:

Somewhat amusing to get to this thread a few minutes after creating a Reitveld issue for the first pass of my urllib.parse patch :)

Hello Nick, could you please point me to that? Also, in general here are my points on Code Review using Rietveld. I think, Martin already indicated that Roundup can post to Rietveld instance. That should really be helpful. I imagine a work-flow like this. 1. Contributor creates a patch and uploads to Roundup tracker. 2. If the patch is very small or does not require review, the committer can just commit it. 3. If the patch does review review, one can click a link to do "Patch Review" wherein only at the moment, the Patch is POSTed the rietveld instance and is available for review. -- Senthil "I went to the museum where they had all the heads and arms from the statues that are in all the other museums." -- Steven Wright

The torrential rains are causing havoc with my internet, so apologies for replying out of sequence. On Sep 30, 2010, at 07:17 PM, Stephen J. Turnbull wrote:

Sorry for following up to myself, but this typo might be very confusing:

Stephen J. Turnbull writes:

...

Barry Warsaw writes:

...

You can have "co-located" branches[1] which essentially switch in-place, so if a branch is changing some .c files, you won't have to rebuild the whole world just to try out a patch.

In Mercurial these are called "named branches", and they are repo-local (by which I mean they must be part of the DAG). Named branches used to have some inconvenient aspects relevant to standalone

s/relevant/relative/

...

branches (they could be fairly confusing to other users if pushed before being merge to mainline).

It's not obvious to me that Mercurial style named branches would work well here ... it would take a little thought to design an appropriate workflow, anyway.

I should note that I don't particularly like colocated/named branches. I personally much prefer separate directories for each feature or bug I'm working on. It helps me keep track of what I'm doing. I have a fast machine so recompiling all of Python is no big deal. I do like having the choice of being able to colocate or not, based on my own workflow preferences. But I suppose with Mercurial I can just have multiple copies of the same branch in different directories, and just start out with "hg update -C foo" -Barry

On 02:47 pm, jnoller@gmail.com wrote:

On Wed, Sep 29, 2010 at 2:32 PM, Guido van Rossum 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.

While I would personally love to see Rietveld declared the official core Python code review tool, I realize that since I wrote as a Google engineer and it is running on Google infrastructure (App Engine), I can't be fully objective about the tool choice -- even though it is open source, has several non-Googler maintainers, and can be run outside App Engine as well.

But I do think that using a specialized code review tool rather than unstructured email plus a general-purpose issue tracker can hugely improve developer performance and also increase community participation. (A code review tool makes it much more convenient for a senior reviewer to impart their wisdom to a junior developer without appearing judgmental or overbearing.)

See also this buzz thread: http://www.google.com/buzz/115212051037621986145/At6Rj82Kret/When- will-the-Python-dev-community-start-using

-- --Guido van Rossum (python.org/~guido)

Regardless of the tool(s) used, code reviews are a fantastic equalizer. If you have long time, experienced developers "submitting" to the same rules that newer contributors have to follow then it helps remove the idea that there is special treatment occurring.

Of course, this is only true if the core developers *do* submit to the same rules. Is anyone proposing that current core committers have all their work reviewed before it is accepted? (I am strongly in favor of this, but I don't think many core committers are.) Jean-Paul

On Thu, Sep 30, 2010 at 7:52 AM, wrote:

On 02:47 pm, jnoller@gmail.com wrote:

...

Regardless of the tool(s) used, code reviews are a fantastic equalizer. If you have long time, experienced developers "submitting" to the same rules that newer contributors have to follow then it helps remove the idea that there is special treatment occurring.

Of course, this is only true if the core developers *do* submit to the same rules.  Is anyone proposing that current core committers have all their work reviewed before it is accepted?

(I am strongly in favor of this, but I don't think many core committers are.)

Having worked in this style for almost 5 years now, I am also strongly in favor. Jesse expressed it better than I could. -- --Guido van Rossum (python.org/~guido)

On Thu, Sep 30, 2010 at 9:52 AM, wrote:

Of course, this is only true if the core developers *do* submit to the same rules. Is anyone proposing that current core committers have all their work reviewed before it is accepted?

I think most would welcome (or at least tolerate ;) ) additional review of their code. The hard part is encouraging contributors to find the time and motivation to thoroughly review code that they aren't personally interested in (and perhaps not even familiar with). -- Daniel Stutzbach, Ph.D. President, Stutzbach Enterprises, LLC http://stutzbachenterprises.com/

The hard part is encouraging contributors to find the time and motivation to thoroughly review code that they aren't personally interested in (and perhaps not even familiar with).

Not sure how well 'tit for tat' schemes work - we *could* require that people don't commit unreviewed changes, and also require that you can't commit unless you have reviewed somebody else's changes. So if you do 10 reviews, you are entitled to 10 commits... Of course, that would put more burden on those people who already do all the work. Regards, Martin

On Thu, Sep 30, 2010 at 10:48 AM, "Martin v. Löwis" wrote:

Not sure how well 'tit for tat' schemes work - we *could* require that people don't commit unreviewed changes, and also require that you can't commit unless you have reviewed somebody else's changes.

I wonder if a "reputation" scheme would work better. Track and publicize patch submissions, reviews, and the review/patch ratio, but do not enforce any particular ratios. Perhaps provide a roundup query showing patches awaiting review sorted by the patch submitter's review/patch ratio? (in descending order) Obviously there would be many non-trivial details to work out. I'm just brainstorming. -- Daniel Stutzbach, Ph.D. President, Stutzbach Enterprises, LLC http://stutzbachenterprises.com/

On Sep 30, 2010, at 10:47 AM, Jesse Noller wrote:

Not to mention; there's a lot to be learned from doing them on both sides. At work, I learn about chunks of code I might not have otherwise known about or approaches to a problem I'd never considered. I sort of drank the kool-aid.

Tools aside, I completely agree. Many projects that I contribute to have policies such as "nothing lands without reviewer approval". For some that means one reviewer must approve it, for others two +1s and no -1s, or a coding approval and a ui approval, etc. When I was the review team lead on Launchpad, I had a goal that every developer would also eventually be a reviewer. We started with a small number of experienced developers, then ran a mentor program to train new reviewers (who we called "mentats"). A mentat approval was not enough to land a branch, but the mentor could basically say "yes, i agree with the review" and it would land. Eventually, by mutual consent a mentat would graduate to full reviewership (and hopefully be a mentor to new reviewers). This was hugely successful among many dimensions. It got everyone on the same page as to coding standards, it equalized the playing field, it got everyone to think collaboratively as a team, folks learned about parts of the system they didn't have day-to-day intimate knowledge about, and it got changes landed much more quickly. Here are a few things we learned along the way. Their applicability to Python will vary of course, and we need to find what works for us. * Keep branches *small*. We had a limit of 800 lines of diff, with special explicit permission from the person reviewing your change to exceed. 800 lines is about the maximum that a person can review in a reasonable amount of time without losing focus. * The *goal* was 5 minutes review, but the reality was that most reviews took about 15-20 minutes. If it's going longer, you weren't doing it right. This meant that there was a level of trust between the reviewer and the dev, so that the basic design had been previously discussed and agreed upon (we mandated pre-implementation chats), etc. A reviewer was there to sanity check the implementation, watch for obvious mistakes, ensure test coverage for the new code, etc. If you were questioning the basic design, you weren't doing a code review. It was okay to reject a change quickly if you found fatal problems. * The primary purpose of a code review was to learn and teach, and in a sense, the measurable increase in quality was a side-effect. What I mean by that is that the review process cannot catch all bugs. It can reduce them, but it's much more valuable to share expertise on how to do things. E.g. "Did you know that if X happens, you won't be decref'ing Y? We like to use goto statements to ensure that all objects are properly refcounted even in the case of exceptional conditions." That's a teaching moment that happens to improve quality. * Reviews are conversations, and it's a two way street. Many times a dev pushed back on one of my suggestions, and we'd have weekly reviewer meetings to hash out coding standards differences. E.g. Do you check for empty sequences with "if len(foo) == 0" or "if not foo"? The team would make those decisions and you'd live by them even if you didn't agree. It's also critical to document those decisions, and a wiki page style guide works very well (especially when you can point to PEP 8 as your basic style guide for example). * Reviews are collaborations. You're both there to get the code landed so work together, not at odds. Try to reach consensus, and don't be afraid to compromise. All code is ultimately owned by everyone and you never know who will have to read it 2 years from now, so keep things simple, clear, and well commented. These are all aesthetics that a reviewer can help with. * As a reviewer ASK QUESTIONS. The best reviews were the ones that asked lots of questions, such as "have you thought about the race conditions this might introduce?" or "what happens when foo is None?" A reviewer doesn't necessarily have to guess or work out every detail. If you don't understand something, ask a question and move on. Let the coder answer it to your satisfaction. As a reviewer, once all your questions are answered, you know you can approve the change. * Keep reviews fast, easy, and fun. I think this is especially true for Python, where we're all volunteers. Keeping it fast, easy, and fun greatly improves the odds that code will be reviewed for the good of the project. * Have a dispute resolution process. If a reviewer and coder can't agree, appeal to a higher authority. As review team leader, I did occasionally have to break ties. * Record the reviewer in the commit messages. We had highly structured commit messages that included the reviewer name, e.g. % commit -m"[r=barry] Bug 12345; fix bad frobnification in Foo.bar()" thus recording in the revision history both the coder and the reviewer, so that we could always ask someone about the change. * Don't let changes get stale. We had a goal that changes would go from ready-to-code (i.e. design and implementation strategy have already been worked out) to landed in 2 days. The longer a change goes before landing, the more stale it gets, the more conflicts you can have, and the less relevant the code becomes. I know this sounds like a lot of process, but it really was fairly lightweight in practice. And that's the most important! Keep things fast, easy, and fun and it'll get done. Also, these ideas evolved after 3 years of experimentation with various approaches, so let it take some time to evolve. And don't be so married to process that you're afraid to ditch steps that are wasteful and don't contribute value to the project. Certainly some of our techniques won't be relevant for Python. For example, we assigned people to do nothing but reviews for one day out of the week (we call it "on-call reviewers"). This worked for us because team velocity was much more important than individual velocity. Even though at first blush, it seemed like you personally were going to be 20% less productive, team productivity skyrocketed because changes were landing much faster, with much less waste built in. This probably won't work for Python where our involvement is not governed by paycheck, but by the whims of our real jobs and life commitments. -Barry

On Thu, Sep 30, 2010 at 12:53 PM, geremy condra wrote:

On Thu, Sep 30, 2010 at 9:33 AM, Barry Warsaw wrote:

...

On Sep 30, 2010, at 10:47 AM, Jesse Noller wrote:

...

Not to mention; there's a lot to be learned from doing them on both sides. At work, I learn about chunks of code I might not have otherwise known about or approaches to a problem I'd never considered. I sort of drank the kool-aid.

Tools aside, I completely agree.

Many projects that I contribute to have policies such as "nothing lands without reviewer approval".  For some that means one reviewer must approve it, for others two +1s and no -1s, or a coding approval and a ui approval, etc.

When I was the review team lead on Launchpad, I had a goal that every developer would also eventually be a reviewer.  We started with a small number of experienced developers, then ran a mentor program to train new reviewers (who we called "mentats").  A mentat approval was not enough to land a branch, but the mentor could basically say "yes, i agree with the review" and it would land.  Eventually, by mutual consent a mentat would graduate to full reviewership (and hopefully be a mentor to new reviewers).

This was hugely successful among many dimensions.  It got everyone on the same page as to coding standards, it equalized the playing field, it got everyone to think collaboratively as a team, folks learned about parts of the system they didn't have day-to-day intimate knowledge about, and it got changes landed much more quickly.

Here are a few things we learned along the way.  Their applicability to Python will vary of course, and we need to find what works for us.

* Keep branches *small*.  We had a limit of 800 lines of diff, with special  explicit permission from the person reviewing your change to exceed.  800  lines is about the maximum that a person can review in a reasonable amount  of time without losing focus.

* The *goal* was 5 minutes review, but the reality was that most reviews took  about 15-20 minutes.  If it's going longer, you weren't doing it right.  This meant that there was a level of trust between the reviewer and the dev,  so that the basic design had been previously discussed and agreed upon (we  mandated pre-implementation chats), etc.  A reviewer was there to sanity  check the implementation, watch for obvious mistakes, ensure test coverage  for the new code, etc.  If you were questioning the basic design, you  weren't doing a code review.  It was okay to reject a change quickly if you  found fatal problems.

* The primary purpose of a code review was to learn and teach, and in a sense,  the measurable increase in quality was a side-effect.  What I mean by that  is that the review process cannot catch all bugs.  It can reduce them, but  it's much more valuable to share expertise on how to do things.  E.g. "Did  you know that if X happens, you won't be decref'ing Y?  We like to use goto  statements to ensure that all objects are properly refcounted even in the  case of exceptional conditions."  That's a teaching moment that happens to  improve quality.

* Reviews are conversations, and it's a two way street.  Many times a dev  pushed back on one of my suggestions, and we'd have weekly reviewer meetings  to hash out coding standards differences.  E.g. Do you check for empty  sequences with "if len(foo) == 0" or "if not foo"?  The team would make  those decisions and you'd live by them even if you didn't agree.  It's also  critical to document those decisions, and a wiki page style guide works very  well (especially when you can point to PEP 8 as your basic style guide for  example).

* Reviews are collaborations.  You're both there to get the code landed so  work together, not at odds.  Try to reach consensus, and don't be afraid to  compromise.  All code is ultimately owned by everyone and you never know who  will have to read it 2 years from now, so keep things simple, clear, and  well commented.  These are all aesthetics that a reviewer can help with.

* As a reviewer ASK QUESTIONS.  The best reviews were the ones that asked lots  of questions, such as "have you thought about the race conditions this might  introduce?" or "what happens when foo is None?"  A reviewer doesn't  necessarily have to guess or work out every detail.  If you don't understand  something, ask a question and move on.  Let the coder answer it to your  satisfaction.  As a reviewer, once all your questions are answered, you know  you can approve the change.

* Keep reviews fast, easy, and fun.  I think this is especially true for  Python, where we're all volunteers.  Keeping it fast, easy, and fun greatly  improves the odds that code will be reviewed for the good of the project.

* Have a dispute resolution process.  If a reviewer and coder can't agree,  appeal to a higher authority.  As review team leader, I did occasionally  have to break ties.

* Record the reviewer in the commit messages.  We had highly structured commit  messages that included the reviewer name, e.g.

 % commit -m"[r=barry] Bug 12345; fix bad frobnification in Foo.bar()"

 thus recording in the revision history both the coder and the reviewer, so  that we could always ask someone about the change.

* Don't let changes get stale.  We had a goal that changes would go from  ready-to-code (i.e. design and implementation strategy have already been  worked out) to landed in 2 days.  The longer a change goes before landing,  the more stale it gets, the more conflicts you can have, and the less  relevant the code becomes.

I know this sounds like a lot of process, but it really was fairly lightweight in practice.  And that's the most important!  Keep things fast, easy, and fun and it'll get done.  Also, these ideas evolved after 3 years of experimentation with various approaches, so let it take some time to evolve. And don't be so married to process that you're afraid to ditch steps that are wasteful and don't contribute value to the project.

Certainly some of our techniques won't be relevant for Python.  For example, we assigned people to do nothing but reviews for one day out of the week (we call it "on-call reviewers").  This worked for us because team velocity was much more important than individual velocity.  Even though at first blush, it seemed like you personally were going to be 20% less productive, team productivity skyrocketed because changes were landing much faster, with much less waste built in.  This probably won't work for Python where our involvement is not governed by paycheck, but by the whims of our real jobs and life commitments.

Extremely well put. Could this kind of process be put in place for the code sprints Jesse's interested in? Seems like an ideal testbed.

Geremy Condra

We *should* encourage this within the Sprints docs/dev guide, etc.

On Thu, Sep 30, 2010 at 09:19, Georg Brandl wrote:

Am 29.09.2010 20:49, schrieb Guido van Rossum:

...

Unfortunately taking the average patch posted to the tracker and importing it in Rietveld is very iffy -- it's very hard to find the right branch+rev needed to be able to apply the patch correctly -- not to mention that there are so many (slightly) different patch formats. It would take a fair bit of AI to get this right.

The recommended way to work with Rietveld is to use a checkout (fortunately with Hg this will become easier to do) and use the "upload.py" script that you can download from Rietveld (log in and click on the Create link).

Then could we do it the other way round and let (our) upload.py open an issue with the patch automatically (after querying for information not already given to rietveld)?

Don't see why not, but those of us who use OpenID would need to start caring about a password which would be unfortunate. -Brett

Georg

-- Thus spake the Lord: Thou shalt indent with four spaces. No more, no less. Four shall be the number of spaces thou shalt indent, and the number of thy indenting shall be four. Eight shalt thou not indent, nor either indent thou two, excepting that thou then proceed to four. Tabs are right out.

_______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/brett%40python.org

"Stephen J. Turnbull" writes:

Barry Warsaw writes:

...

You can have "co-located" branches[1] which essentially switch in-place, so if a branch is changing some .c files, you won't have to rebuild the whole world just to try out a patch.

In Mercurial these are called "named branches", and they are repo-local (by which I mean they must be part of the DAG).

Think of it this way: you can have several clones and when you make different commits in each, they naturally begin to diverge: clone 1: ... [X] --- [A] --- [B] clone 2: ... [X] --- [R] clone 3: ... [X] --- [U] --- [V] Though you cannot see it yet, you have created branches in the history. If you pull one clone 2 into clone 1, then the branch suddenly becomes visible in that clone -- you have two "heads" (B and R): clone 1: ... [X] --- [A] --- [B] \ [R] You can of course pull in clone 3 as well: clone 1: ... [X] --- [A] --- [B] | \ | [R] \ [U] --- [V] You can now use 'hg update' to switch between these three anonymous branches. You can find the three heads by using 'hg heads' or you can use the bookmarks extension to assign labels to them. These labels will move forward when you commit, just like you move a bookmark forward in a book when you read through it. Btw, you can separate such a repository again with 'hg clone -r' which lets you ask for a clone containing just a subset of another repository. You can of course also split off named branches (see below) in this way.

Named branches used to have some inconvenient aspects relevant to standalone branches (they could be fairly confusing to other users if pushed before being merge to mainline).

Now, "named branches" is sort of an extension of the above scenario. Along with the commit message, date, username, etc..., each changeset in Mercurial contains a label that tells us which "named branch" it belongs to. By default, changesets are on the branch called, well, "default". Named branches allow you to group or namespace your changesets. For Mercurial itself, we use two named branches: default and stable. Bugfixes go on the stable branch and we do our development on the default branch. Though a named branch is a collection of changesets, you often refer to a named branch in a context where you need just a single changeset. It is then interpreted as the tip-most changeset with that branch name. So $ hg update stable will checkout the latest (tip-most) changeset on the stable branch. If you now commit a bugfix, then that changeset will also be on the stable branch since the branch name is inherited from the parent changeset. In Mercurial we then merge the stable branch back into the default branch: # fix bug $ hg commit -m 'Fixed issue 123' $ hg update default $ hg merge stable $ hg commit -m 'Merged in fix for issue123' I've written a guide to such a workflow here: http://mercurial.aragost.com/kick-start/tasks.html

It's not obvious to me that Mercurial style named branches would work well here ... it would take a little thought to design an appropriate workflow, anyway.

Yeah, you guys should try it out first -- it's easy to introduce named branches later if you want. -- Martin Geisler Mercurial links: http://mercurial.ch/

Barry Warsaw writes:

I should note that I don't particularly like colocated/named branches. I personally much prefer separate directories for each feature or bug I'm working on. It helps me keep track of what I'm doing. I have a fast machine so recompiling all of Python is no big deal.

Note that once you have a branch with all of Python built, for any of hg, git, bzr you may prefer to clone the branch/repo with "cp -r" or "rsync" rather than "$VCS clone". The reason for doing it with "$VCS clone" is to get a pristine workspace without any cruft like editor backups or build products. If you *want* a crufty workspace (and in this case some people do), a recursive copy is a better tool.

Georg Brandl writes:

Am 30.09.2010 10:22, schrieb Dirkjan Ochtman:

...

On Wed, Sep 29, 2010 at 20:32, Guido van Rossum 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.

Rambling thoughts about some of the things mentioned in this thread.

I think hg-review looks interesting, though it may not (yet) have the level of sophistication of Rietveld. (Public test instance at http://review.stevelosh.com/.)

It might be interesting to integrate Rietveld uploads in a Mercurial extension, particularly if it gets integrated with mq somehow.

That would be totally awesome!

The Go (the language) project has a Mercurial extension that integrates Rietveld. It seems to provide a few commands for both directions: uploading changelists to a review server and for the reviewer downloading from the server and applying the changelists to a working copy. I never used this extension myself so I can't tell anything about the workflow introduced by these commands. The sources for this extension are here: http://code.google.com/p/go/source/browse/lib/codereview/codereview.py Andi

Georg

On Sep 30, 2010, at 01:46 PM, Brett Cannon wrote:

Once we have a good workflow in place we would have to start shifting our development culture towards requiring a review of code no matter who the author is (which I support doing).

I should note one other thing, in reference to my previous posting about reviews. Launchpad does have a backdoor for getting changes in without formal review. It's called "rubber stamping" and shows up in commit messages, e.g.: $VCS commit -m"[rs=me] Fix trivial misspelling in comment" You can also get a rubber stamp from a reviewer: Alice: can you review my branch that fixes all incorrect uses of "it's"? Bob: If that's your only change, I trust you. rs=me Alice: $VCS commit -m"[rs=bob] The Grammar Nanny strikes again" Usually rubber stamps are reserved for cases where the fix really is trivial, or a change is large but mechanical, or when no reviewer can be found for a time-sensitive fix (very rare). You at least need to record the rubber stamp in the commit message, and be prepared to defend it if it trips up someone's post-commit eyeball filter. -Barry

On Sat, Oct 2, 2010 at 2:31 AM, Barry Warsaw wrote:

Usually rubber stamps are reserved for cases where the fix really is trivial, or a change is large but mechanical, or when no reviewer can be found for a time-sensitive fix (very rare).  You at least need to record the rubber stamp in the commit message, and be prepared to defend it if it trips up someone's post-commit eyeball filter.

A system like that, which still trusts committers to make the call that rubber stamping is appropriate, sounds significantly more workable to me than one which required review even for trivial changes. Cheers, Nick. -- Nick Coghlan   |   ncoghlan@gmail.com   |   Brisbane, Australia

Am 02.10.2010 22:42, schrieb Jesus Cea:

On 30/09/10 22:41, Brett Cannon wrote:

...

Don't see why not, but those of us who use OpenID would need to start caring about a password which would be unfortunate.

+1. OpenID or OAuth is a must.

Moreover, I am a bit worried of needing a google account. Google already knows too much about me, I don't want to navigate with a google cookie in my browser.

Guido says that Rietveld can run outside of Google. That is not a bad option. I could host it, I guess, but my server uptime is only around 99.7% (half an hour per week).

Don't worry. It's hosted on bugs.python.org now. Regards, Martin