We should be using a tool for code reviews
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-P... -- --Guido van Rossum (python.org/~guido)
On Wed, 29 Sep 2010 11:32:19 -0700 Guido van Rossum <guido@python.org> wrote:
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, Antoine Pitrou <solipsis@pitrou.net> wrote:
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?).
On Wed, Sep 29, 2010 at 11:47 AM, Brett Cannon <brett@python.org> wrote:
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.
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 Wed, Sep 29, 2010 at 12:03, Guido van Rossum <guido@python.org> wrote:
Well, I would assume people are working from a checkout. Patches from an outdated checkout simply would fail and that's fine by me.
That's fine by me.
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 1:12 PM, Brett Cannon <brett@python.org> wrote:
Ok, but that's an extra barrier for contributions. Lots of people when asked for a patch just modify their distro in place and you can count yourself lucky if they send you a diff from a clean copy. But maybe with Hg it's less of a burden to ask people to use a checkout.
How often do we even get patches generated from a downloaded copy of Python? Is it enough to need to worry about this?
I used to get these frequently. I don't know what the experience of the current crop of core developers is though, so maybe my gut feelings here are outdated. -- --Guido van Rossum (python.org/~guido)
On Wed, Sep 29, 2010 at 4:23 PM, Guido van Rossum <guido@python.org> 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 9/29/10 3:33 PM, Guido van Rossum wrote:
For what it's worth, when I tried it, the relationship is reversed: [hg]$ time hg clone http://code.python.org/hg/branches/py3k/ ... hg clone http://code.python.org/hg/branches/py3k/ 24.44s user 3.43s system 10% cpu 4:15.48 total [hg]$ du -hsc py3k 105M py3k [svn]$ time svn co http://svn.python.org/projects/python/branches/py3k/ ... svn co http://svn.python.org/projects/python/branches/py3k/ 5.03s user 7.01s system 12% cpu 1:35.97 total [svn]$ du -hsc py3k 131M py3k Mercurial's checkout with the whole history is actually smaller than the SVN checkout because it applies some very nice compression to the history whereas the SVN checkout has an uncompressed copy of every single file tucked away in its .svn/ directory. My mercurial checkout happens to be slower, but I don't know whose fault that is. I'm still using Mercurial 1.5.1 on my OS X box, and while I wasn't doing much that would take up CPU or bandwidth, I wasn't trying especially hard to prevent such interference, either. -- Robert Kern "I have come to believe that the whole world is an enigma, a harmless enigma that is made terrible by our own mad attempt to interpret it as though it had an underlying truth." -- Umberto Eco
On Wed, Sep 29, 2010 at 13:31, Alexander Belopolsky <alexander.belopolsky@gmail.com> 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). 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. 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.
On Wed, 29 Sep 2010 13:56:46 -0700 Brett Cannon <brett@python.org> wrote:
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.
On Wed, Sep 29, 2010 at 4:56 PM, Brett Cannon <brett@python.org> wrote:
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.
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.
On Sep 29, 2010, at 05:22 PM, Alexander Belopolsky wrote:
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).
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
Well, either they're doing a small patch and uploading to Rietveld doesn't bring much anyway; or they're doing a large patch and it may not apply cleanly if they do it against the stable release. So I don't think there's a real problem here.
Most patches posted on the tracker are generated either by "SVN diff" or a DCVS equivalent (often hg). 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 Wed, 29 Sep 2010 17:30:10 -0400 Barry Warsaw <barry@python.org> wrote:
When I review a patch, I will tend to assume that the poster has already run the test suite (especially if it's another committer). Having to checkout a branch and generate a diff myself would make it much longer to produce a review, in most cases. Even rebuilding a new branch from scratch can be slower than applying the diff in an existing checkout and letting `make` rebuild a couple of files. Regards Antoine.
On Sep 30, 2010, at 12:04 AM, Antoine Pitrou wrote:
Yep, it depends on who is submitting the branch, what the branch changes, etc.
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. -Barry [1] I only have experience with these in Bazaar so Mercurial's might work differently or be called something different.
Barry Warsaw writes:
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.
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:
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
Barry Warsaw writes:
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.
On Oct 01, 2010, at 01:09 PM, Stephen J. Turnbull wrote:
Yep, absolutely agree. I also use 'cp -a' or 'rsync -avz' when I want that crufty workspace transfered to another (local) machine. In general though, when I'm working on feature or bug branches, I want a pristine working directory. -Barry
"Stephen J. Turnbull" <stephen@xemacs.org> writes:
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.
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
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/
On Wed, Sep 29, 2010 at 13:23, Guido van Rossum <guido@python.org> wrote:
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. From there we can see if there is a drop in submissions.
Le mercredi 29 septembre 2010 à 13:35 -0700, Brett Cannon a écrit :
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 <solipsis@pitrou.net> wrote:
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 <solipsis@pitrou.net> wrote:
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."
2010/9/29 Guido van Rossum <guido@python.org>:
FWIW, I usually don't apply patches until the last stages of review (namely I'm going to apply it). So, it's a little annoying to download and apply a patch just to run upload.py. I'd prefer a pure web interface for starting reviews with a code review tool. -- Regards, Benjamin
On Wed, Sep 29, 2010 at 01:23:24PM -0700, Guido van Rossum wrote:
When helping out on a Linux distribution, dealing with patches against the latest tarball is a fairly frequent occurrence. The question would be whether these patches get filtered through the maintainer of the package before landing in roundup/rietveld and whether the distro maintainer is sufficiently in tune with python development that they're maintaining both patches against the last tarball and a checkout of trunk with the patches applied intelligently there. A few other random thoughts: * hg could be more of a burden in that it may be unfamiliar to the casual python user who happens to have found a fix for a bug and wants to submit it. cvs and svn are similar enough that people comfortable with one are usually comfortable with the other but hg has different semantics. * The barrier to entry seems to be higher the less well integrated the tools are. I occassionally try to contribute patches to bzr in launchpad and the integration there is horrid. You end up with two separate streams of comments and you don't automatically get subscribed to both. There's several UI elements for associating a branch with a bug but some of them are buggy (or else are very strict on what input they're expecting) while other ones are hard to find. Since I only contribute a patch two or three times a year, I have to re-figure out the process each time I try to contribute. * I like the idea of patch complexity being a measure of whether the patch needs to go into a code review tool in that it keeps simple things simple and gives more advanced tools to more advanced cases. I dislike it in that for someone who's just contributing a patch to fix a problem that they're encountering which happens to be somewhat complex, they end up having to learn a lot about tools that they may never use again. * It seems like code review will be a great aid to people who submit changes or review changes frequently. The trick will be making it non-intimidating for someone who's just going to contribute changes infrequently. -Toshio
On 29 September 2010 21:12, Brett Cannon <brett@python.org> wrote:
How often do we even get patches generated from a downloaded copy of Python? Is it enough to need to worry about this?
When I do simple bugfixes of library code, I'll often work from my "live" Python environment, patch it in place, test and generate a patch with diff. Very primitive, but surprisingly effective. Having said that, I don't think it's the end of the world if such patches couldn't use Reitveld. They are probably small enough that it would be overkill anyway. Paul
Guido, Brett, On Wed, 29 Sep 2010 11:47:51 -0700 Brett Cannon <brett@python.org> wrote:
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.
It sounds like a good option (or, even better, a customized version as suggested by Daniel).
We shouldn't require it. Some people don't have a Google account (and/or don't want to have one). Also sometimes posting on Rietveld is overkill (patches less than 30 lines long fall in this category, as far as I'm concerned).
Yes, I think Ezio was looking into it, but working on the tracker apparently has a tendency to burn out developers rather quickly :-/ (first Ajaksu, now Ezio) Regards Antoine.
On Thu, Sep 30, 2010 at 4:47 AM, Brett Cannon <brett@python.org> 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 :)
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 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
On Wed, Sep 29, 2010 at 11:41 AM, Antoine Pitrou <solipsis@pitrou.net> wrote:
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)
Am 29.09.2010 20:49, schrieb Guido van Rossum:
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)? 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.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 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). - -- Jesus Cea Avion _/_/ _/_/_/ _/_/_/ jcea@jcea.es - http://www.jcea.es/ _/_/ _/_/ _/_/ _/_/ _/_/ jabber / xmpp:jcea@jabber.org _/_/ _/_/ _/_/_/_/_/ . _/_/ _/_/ _/_/ _/_/ _/_/ "Things are not so easy" _/_/ _/_/ _/_/ _/_/ _/_/ _/_/ "My name is Dump, Core Dump" _/_/_/ _/_/_/ _/_/ _/_/ "El amor es poner tu felicidad en la felicidad de otro" - Leibniz -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQCVAwUBTKeZI5lgi5GaxT1NAQLcRgP/YldvoSubtVYUC3IxdXtC+XjiKWaC28eJ xIp3CxWwtuzAGrYl/kfiyrxLpk40jL/T6xEdU8r/lXXxKttbBBThLsNf93LrECCB 4uxmIdEY+SkK5Mj2gU3FWTQ4PQfRspAwYtfjGvnaPEbVLTWOccqAK4SsyEpkZ2n9 wQRUNYURv44= =qoXI -----END PGP SIGNATURE-----
On Wed, Sep 29, 2010 at 1:41 PM, Antoine Pitrou <solipsis@pitrou.net> wrote:
Most patches won't import cleanly into Rietveld, because the patch typical does not contain precise information about the base revision for the patch. How about the opposite approach: make a Python-specific version of upload.py that lets the user attach the patch to an issue with an optional message? -- Daniel Stutzbach, Ph.D. President, Stutzbach Enterprises, LLC <http://stutzbachenterprises.com/>
On Sep 29, 2010, at 11:32 AM, Guido van Rossum wrote:
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!
Guido> I would like to recommend that the Python core developers start Guido> using a code review tool ... +1 I've suggested that something like Rietveld be integrated with our Roundup instance in the past. I suspect there is an open tracker case. Martin will be better able to find it than me. I became a convert watching the Unladen Swallow folks use it. Skip
I considered having roundup post all patches to Rietveld (trust me that I can solve the "what branch and revision problem"). However, the stumbling block is access control - I don't know how to get roundup to post to Rietveld, "anonymously", so to speak. So perhaps we should just run our own Rietveld instance next to Roundup. Regards, Martin
On Wed, Sep 29, 2010 at 14:35, "Martin v. Löwis" <martin@v.loewis.de> 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. 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 3:12 PM, Antoine Pitrou <solipsis@pitrou.net> wrote:
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 Thu, Sep 30, 2010 at 7:35 AM, "Martin v. Löwis" <martin@v.loewis.de> 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? Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
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 5:57 PM, Nick Coghlan <ncoghlan@gmail.com> wrote:
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 20:32, Guido van Rossum <guido@python.org> wrote:
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
Am 30.09.2010 10:22, schrieb Dirkjan Ochtman:
That would be totally awesome! 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.
Georg Brandl <g.brandl@gmx.net> writes:
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
Hi, On using code review tools: +1, no discussion. I've recently been doing a bit of research on these as a side effect of researching continuous deployment, so: 1. Barry is right about Launchpad's merge proposals (unsurprisingly) 2. hg has a review extension called hg-review, but I think it'll be too difficult to use properly for a big dev team like Python's with many people reading code reviews (hg-review stores reviews in the repository) -- there's no real canonical way AFAIK of saying "give me all pending reviews everywhere in the codebase", like you would have with a centralized place to publish patches vs a specific revision. (I *am* going to use hg-review personally for my startup, I'm not saying it's a bad tool at all! Just that it's not very good for big teams yet, because there's no real sensible way of getting a centralized UI for both publishing and reviewing suggested patches.) 3. FWIW, I agree Rietveld's a better tool than ReviewBoard, and I'm not attached to either of the authors, so Rietveld +1 cheers lvh
On Wed, Sep 29, 2010 at 2:32 PM, Guido van Rossum <guido@python.org> 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. Additionally, a lot of people are terrified of code reviews as they view it as a "public flogging" - holding everyone to the same standards, and showing this is not the case helps fight this perception. 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.
On 02:47 pm, jnoller@gmail.com 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 am strongly in favor of this, but I don't think many core committers are.) Jean-Paul
On Thu, Sep 30, 2010 at 10:52 AM, <exarkun@twistedmatrix.com> wrote:
I'll propose it, knowing full well I won't win. Code reviews have saved my bacon on numerous occasions. The best unit tests on the planet won't protect you against a fundamentally bad assumption or logic error. Like I said - I think it helps "equalize" things. YMMV.
On Fri, Oct 1, 2010 at 12:56 AM, Guido van Rossum <guido@python.org> wrote:
I'll be one of those to object (but only slightly). I think one of the privileges/responsibilities that goes with commit access is the ability to make the call between: - "this is a simple change/fix, I'll just check it in with possible post hoc review via python-checkins" - "I want feedback on the idea and/or details before I commit this, I'll post a patch for review to the tracker" - "I may want help in getting this working and/or this may take a while to get right, so I'll create a branch for it" (with the balance between 2 and 3 apparently shifting more in favour of 3 once we have hg to play with) Particularly for user visible API changes, I think getting a sanity check from at least one other dev before committing is a good idea. For smaller stuff, I think python-checkins after the fact reviews are enough to cover it (particularly now that one person asking a question will kick the entire diff over to python-dev for broader review). Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Thu, 30 Sep 2010 14:52:18 -0000 exarkun@twistedmatrix.com wrote:
While I agree with non-trivial patches being *posted* for review, I don't agree that an actual review should happen in order for a patch to be committed. That's unless we get 10x the number of reviewers we currently have. (and you certainly know by experience that I would be glad to have reviews on my patches, especially the network-related ones ;-)) Regards Antoine.
On Thu, Sep 30, 2010 at 9:52 AM, <exarkun@twistedmatrix.com> wrote:
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/>
Am 30.09.2010 17:40, schrieb Senthil Kumaran:
I think the supporters of code review readily accept that: in return for the higher effort, they say, we also get more quality. So it's really a quality vs. quantity thing. It would be easy to just commit all patches that are uploaded to roundup, but of course, it would horribly break Python. With mandatory code review, even less patches get reviewed than today. Regards, Martin
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" <martin@v.loewis.de>wrote:
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 Thu, Sep 30, 2010 at 10:31, Daniel Stutzbach < daniel@stutzbachenterprises.com> wrote:
I definitely welcome additional, or in some cases, *any* review. Looking for reviews of Windows features/bugs sometimes equates to looking in a ghost town, but I have the self-inflicted problem of using Windows in the first place ;) Anyways, a big +1 to expanding review, especially incorporating something like Rietveld. Although I'm replying out of order, Barry's big response lays out a lot of good ideas that I think we can use.
On Thu, Sep 30, 2010 at 08:31, Daniel Stutzbach <daniel@stutzbachenterprises.com> 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). As for needing some point system or incentive to drive people to do it, I don't think that would be necessary. We are lucky to have a dev team that is friendly and respectful. I suspect that e.g., if Nick started doing reviews of importlib patches I would reciprocate the favour by reviewing runpy patches. Those of us who don't do reviews would most likely then just end up with patches sitting in the tracker waiting for a review.
On Sep 30, 2010, at 01:46 PM, Brett Cannon wrote:
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 Fri, Oct 1, 2010 at 12:31 PM, Barry Warsaw <barry@python.org> wrote:
This makes a lot of sense; having to branch & get spelling corrections in docs or comments would be a *huge* pain, and not provide value. -Fred -- Fred L. Drake, Jr. <fdrake at acm.org> "A storm broke loose in my mind." --Albert Einstein
On Sat, Oct 2, 2010 at 2:31 AM, Barry Warsaw <barry@python.org> wrote:
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
On Sep 30, 2010, at 10:47 AM, Jesse Noller wrote:
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 Wed, 29 Sep 2010 11:32:19 -0700 Guido van Rossum <guido@python.org> wrote:
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, Antoine Pitrou <solipsis@pitrou.net> wrote:
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?).
On Wed, Sep 29, 2010 at 11:47 AM, Brett Cannon <brett@python.org> wrote:
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.
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 Wed, Sep 29, 2010 at 12:03, Guido van Rossum <guido@python.org> wrote:
Well, I would assume people are working from a checkout. Patches from an outdated checkout simply would fail and that's fine by me.
That's fine by me.
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 1:12 PM, Brett Cannon <brett@python.org> wrote:
Ok, but that's an extra barrier for contributions. Lots of people when asked for a patch just modify their distro in place and you can count yourself lucky if they send you a diff from a clean copy. But maybe with Hg it's less of a burden to ask people to use a checkout.
How often do we even get patches generated from a downloaded copy of Python? Is it enough to need to worry about this?
I used to get these frequently. I don't know what the experience of the current crop of core developers is though, so maybe my gut feelings here are outdated. -- --Guido van Rossum (python.org/~guido)
On Wed, Sep 29, 2010 at 4:23 PM, Guido van Rossum <guido@python.org> 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 9/29/10 3:33 PM, Guido van Rossum wrote:
For what it's worth, when I tried it, the relationship is reversed: [hg]$ time hg clone http://code.python.org/hg/branches/py3k/ ... hg clone http://code.python.org/hg/branches/py3k/ 24.44s user 3.43s system 10% cpu 4:15.48 total [hg]$ du -hsc py3k 105M py3k [svn]$ time svn co http://svn.python.org/projects/python/branches/py3k/ ... svn co http://svn.python.org/projects/python/branches/py3k/ 5.03s user 7.01s system 12% cpu 1:35.97 total [svn]$ du -hsc py3k 131M py3k Mercurial's checkout with the whole history is actually smaller than the SVN checkout because it applies some very nice compression to the history whereas the SVN checkout has an uncompressed copy of every single file tucked away in its .svn/ directory. My mercurial checkout happens to be slower, but I don't know whose fault that is. I'm still using Mercurial 1.5.1 on my OS X box, and while I wasn't doing much that would take up CPU or bandwidth, I wasn't trying especially hard to prevent such interference, either. -- Robert Kern "I have come to believe that the whole world is an enigma, a harmless enigma that is made terrible by our own mad attempt to interpret it as though it had an underlying truth." -- Umberto Eco
On Wed, Sep 29, 2010 at 13:31, Alexander Belopolsky <alexander.belopolsky@gmail.com> 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). 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. 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.
On Wed, 29 Sep 2010 13:56:46 -0700 Brett Cannon <brett@python.org> wrote:
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.
On Wed, Sep 29, 2010 at 4:56 PM, Brett Cannon <brett@python.org> wrote:
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.
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.
On Sep 29, 2010, at 05:22 PM, Alexander Belopolsky wrote:
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).
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
Well, either they're doing a small patch and uploading to Rietveld doesn't bring much anyway; or they're doing a large patch and it may not apply cleanly if they do it against the stable release. So I don't think there's a real problem here.
Most patches posted on the tracker are generated either by "SVN diff" or a DCVS equivalent (often hg). 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 Wed, 29 Sep 2010 17:30:10 -0400 Barry Warsaw <barry@python.org> wrote:
When I review a patch, I will tend to assume that the poster has already run the test suite (especially if it's another committer). Having to checkout a branch and generate a diff myself would make it much longer to produce a review, in most cases. Even rebuilding a new branch from scratch can be slower than applying the diff in an existing checkout and letting `make` rebuild a couple of files. Regards Antoine.
On Sep 30, 2010, at 12:04 AM, Antoine Pitrou wrote:
Yep, it depends on who is submitting the branch, what the branch changes, etc.
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. -Barry [1] I only have experience with these in Bazaar so Mercurial's might work differently or be called something different.
Barry Warsaw writes:
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.
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:
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
Barry Warsaw writes:
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.
On Oct 01, 2010, at 01:09 PM, Stephen J. Turnbull wrote:
Yep, absolutely agree. I also use 'cp -a' or 'rsync -avz' when I want that crufty workspace transfered to another (local) machine. In general though, when I'm working on feature or bug branches, I want a pristine working directory. -Barry
"Stephen J. Turnbull" <stephen@xemacs.org> writes:
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.
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
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/
On Wed, Sep 29, 2010 at 13:23, Guido van Rossum <guido@python.org> wrote:
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. From there we can see if there is a drop in submissions.
Le mercredi 29 septembre 2010 à 13:35 -0700, Brett Cannon a écrit :
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 <solipsis@pitrou.net> wrote:
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 <solipsis@pitrou.net> wrote:
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."
2010/9/29 Guido van Rossum <guido@python.org>:
FWIW, I usually don't apply patches until the last stages of review (namely I'm going to apply it). So, it's a little annoying to download and apply a patch just to run upload.py. I'd prefer a pure web interface for starting reviews with a code review tool. -- Regards, Benjamin
On Wed, Sep 29, 2010 at 01:23:24PM -0700, Guido van Rossum wrote:
When helping out on a Linux distribution, dealing with patches against the latest tarball is a fairly frequent occurrence. The question would be whether these patches get filtered through the maintainer of the package before landing in roundup/rietveld and whether the distro maintainer is sufficiently in tune with python development that they're maintaining both patches against the last tarball and a checkout of trunk with the patches applied intelligently there. A few other random thoughts: * hg could be more of a burden in that it may be unfamiliar to the casual python user who happens to have found a fix for a bug and wants to submit it. cvs and svn are similar enough that people comfortable with one are usually comfortable with the other but hg has different semantics. * The barrier to entry seems to be higher the less well integrated the tools are. I occassionally try to contribute patches to bzr in launchpad and the integration there is horrid. You end up with two separate streams of comments and you don't automatically get subscribed to both. There's several UI elements for associating a branch with a bug but some of them are buggy (or else are very strict on what input they're expecting) while other ones are hard to find. Since I only contribute a patch two or three times a year, I have to re-figure out the process each time I try to contribute. * I like the idea of patch complexity being a measure of whether the patch needs to go into a code review tool in that it keeps simple things simple and gives more advanced tools to more advanced cases. I dislike it in that for someone who's just contributing a patch to fix a problem that they're encountering which happens to be somewhat complex, they end up having to learn a lot about tools that they may never use again. * It seems like code review will be a great aid to people who submit changes or review changes frequently. The trick will be making it non-intimidating for someone who's just going to contribute changes infrequently. -Toshio
On 29 September 2010 21:12, Brett Cannon <brett@python.org> wrote:
How often do we even get patches generated from a downloaded copy of Python? Is it enough to need to worry about this?
When I do simple bugfixes of library code, I'll often work from my "live" Python environment, patch it in place, test and generate a patch with diff. Very primitive, but surprisingly effective. Having said that, I don't think it's the end of the world if such patches couldn't use Reitveld. They are probably small enough that it would be overkill anyway. Paul
Guido, Brett, On Wed, 29 Sep 2010 11:47:51 -0700 Brett Cannon <brett@python.org> wrote:
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.
It sounds like a good option (or, even better, a customized version as suggested by Daniel).
We shouldn't require it. Some people don't have a Google account (and/or don't want to have one). Also sometimes posting on Rietveld is overkill (patches less than 30 lines long fall in this category, as far as I'm concerned).
Yes, I think Ezio was looking into it, but working on the tracker apparently has a tendency to burn out developers rather quickly :-/ (first Ajaksu, now Ezio) Regards Antoine.
On Thu, Sep 30, 2010 at 4:47 AM, Brett Cannon <brett@python.org> 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 :)
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 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
On Wed, Sep 29, 2010 at 11:41 AM, Antoine Pitrou <solipsis@pitrou.net> wrote:
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)
Am 29.09.2010 20:49, schrieb Guido van Rossum:
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)? 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.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 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). - -- Jesus Cea Avion _/_/ _/_/_/ _/_/_/ jcea@jcea.es - http://www.jcea.es/ _/_/ _/_/ _/_/ _/_/ _/_/ jabber / xmpp:jcea@jabber.org _/_/ _/_/ _/_/_/_/_/ . _/_/ _/_/ _/_/ _/_/ _/_/ "Things are not so easy" _/_/ _/_/ _/_/ _/_/ _/_/ _/_/ "My name is Dump, Core Dump" _/_/_/ _/_/_/ _/_/ _/_/ "El amor es poner tu felicidad en la felicidad de otro" - Leibniz -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQCVAwUBTKeZI5lgi5GaxT1NAQLcRgP/YldvoSubtVYUC3IxdXtC+XjiKWaC28eJ xIp3CxWwtuzAGrYl/kfiyrxLpk40jL/T6xEdU8r/lXXxKttbBBThLsNf93LrECCB 4uxmIdEY+SkK5Mj2gU3FWTQ4PQfRspAwYtfjGvnaPEbVLTWOccqAK4SsyEpkZ2n9 wQRUNYURv44= =qoXI -----END PGP SIGNATURE-----
On Wed, Sep 29, 2010 at 1:41 PM, Antoine Pitrou <solipsis@pitrou.net> wrote:
Most patches won't import cleanly into Rietveld, because the patch typical does not contain precise information about the base revision for the patch. How about the opposite approach: make a Python-specific version of upload.py that lets the user attach the patch to an issue with an optional message? -- Daniel Stutzbach, Ph.D. President, Stutzbach Enterprises, LLC <http://stutzbachenterprises.com/>
On Sep 29, 2010, at 11:32 AM, Guido van Rossum wrote:
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!
Guido> I would like to recommend that the Python core developers start Guido> using a code review tool ... +1 I've suggested that something like Rietveld be integrated with our Roundup instance in the past. I suspect there is an open tracker case. Martin will be better able to find it than me. I became a convert watching the Unladen Swallow folks use it. Skip
I considered having roundup post all patches to Rietveld (trust me that I can solve the "what branch and revision problem"). However, the stumbling block is access control - I don't know how to get roundup to post to Rietveld, "anonymously", so to speak. So perhaps we should just run our own Rietveld instance next to Roundup. Regards, Martin
On Wed, Sep 29, 2010 at 14:35, "Martin v. Löwis" <martin@v.loewis.de> 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. 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 3:12 PM, Antoine Pitrou <solipsis@pitrou.net> wrote:
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 Thu, Sep 30, 2010 at 7:35 AM, "Martin v. Löwis" <martin@v.loewis.de> 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? Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
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 5:57 PM, Nick Coghlan <ncoghlan@gmail.com> wrote:
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 20:32, Guido van Rossum <guido@python.org> wrote:
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
Am 30.09.2010 10:22, schrieb Dirkjan Ochtman:
That would be totally awesome! 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.
Georg Brandl <g.brandl@gmx.net> writes:
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
Hi, On using code review tools: +1, no discussion. I've recently been doing a bit of research on these as a side effect of researching continuous deployment, so: 1. Barry is right about Launchpad's merge proposals (unsurprisingly) 2. hg has a review extension called hg-review, but I think it'll be too difficult to use properly for a big dev team like Python's with many people reading code reviews (hg-review stores reviews in the repository) -- there's no real canonical way AFAIK of saying "give me all pending reviews everywhere in the codebase", like you would have with a centralized place to publish patches vs a specific revision. (I *am* going to use hg-review personally for my startup, I'm not saying it's a bad tool at all! Just that it's not very good for big teams yet, because there's no real sensible way of getting a centralized UI for both publishing and reviewing suggested patches.) 3. FWIW, I agree Rietveld's a better tool than ReviewBoard, and I'm not attached to either of the authors, so Rietveld +1 cheers lvh
On Wed, Sep 29, 2010 at 2:32 PM, Guido van Rossum <guido@python.org> 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. Additionally, a lot of people are terrified of code reviews as they view it as a "public flogging" - holding everyone to the same standards, and showing this is not the case helps fight this perception. 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.
On 02:47 pm, jnoller@gmail.com 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 am strongly in favor of this, but I don't think many core committers are.) Jean-Paul
On Thu, Sep 30, 2010 at 10:52 AM, <exarkun@twistedmatrix.com> wrote:
I'll propose it, knowing full well I won't win. Code reviews have saved my bacon on numerous occasions. The best unit tests on the planet won't protect you against a fundamentally bad assumption or logic error. Like I said - I think it helps "equalize" things. YMMV.
On Fri, Oct 1, 2010 at 12:56 AM, Guido van Rossum <guido@python.org> wrote:
I'll be one of those to object (but only slightly). I think one of the privileges/responsibilities that goes with commit access is the ability to make the call between: - "this is a simple change/fix, I'll just check it in with possible post hoc review via python-checkins" - "I want feedback on the idea and/or details before I commit this, I'll post a patch for review to the tracker" - "I may want help in getting this working and/or this may take a while to get right, so I'll create a branch for it" (with the balance between 2 and 3 apparently shifting more in favour of 3 once we have hg to play with) Particularly for user visible API changes, I think getting a sanity check from at least one other dev before committing is a good idea. For smaller stuff, I think python-checkins after the fact reviews are enough to cover it (particularly now that one person asking a question will kick the entire diff over to python-dev for broader review). Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Thu, 30 Sep 2010 14:52:18 -0000 exarkun@twistedmatrix.com wrote:
While I agree with non-trivial patches being *posted* for review, I don't agree that an actual review should happen in order for a patch to be committed. That's unless we get 10x the number of reviewers we currently have. (and you certainly know by experience that I would be glad to have reviews on my patches, especially the network-related ones ;-)) Regards Antoine.
On Thu, Sep 30, 2010 at 9:52 AM, <exarkun@twistedmatrix.com> wrote:
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/>
Am 30.09.2010 17:40, schrieb Senthil Kumaran:
I think the supporters of code review readily accept that: in return for the higher effort, they say, we also get more quality. So it's really a quality vs. quantity thing. It would be easy to just commit all patches that are uploaded to roundup, but of course, it would horribly break Python. With mandatory code review, even less patches get reviewed than today. Regards, Martin
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" <martin@v.loewis.de>wrote:
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 Thu, Sep 30, 2010 at 10:31, Daniel Stutzbach < daniel@stutzbachenterprises.com> wrote:
I definitely welcome additional, or in some cases, *any* review. Looking for reviews of Windows features/bugs sometimes equates to looking in a ghost town, but I have the self-inflicted problem of using Windows in the first place ;) Anyways, a big +1 to expanding review, especially incorporating something like Rietveld. Although I'm replying out of order, Barry's big response lays out a lot of good ideas that I think we can use.
On Thu, Sep 30, 2010 at 08:31, Daniel Stutzbach <daniel@stutzbachenterprises.com> 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). As for needing some point system or incentive to drive people to do it, I don't think that would be necessary. We are lucky to have a dev team that is friendly and respectful. I suspect that e.g., if Nick started doing reviews of importlib patches I would reciprocate the favour by reviewing runpy patches. Those of us who don't do reviews would most likely then just end up with patches sitting in the tracker waiting for a review.
On Sep 30, 2010, at 01:46 PM, Brett Cannon wrote:
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 Fri, Oct 1, 2010 at 12:31 PM, Barry Warsaw <barry@python.org> wrote:
This makes a lot of sense; having to branch & get spelling corrections in docs or comments would be a *huge* pain, and not provide value. -Fred -- Fred L. Drake, Jr. <fdrake at acm.org> "A storm broke loose in my mind." --Albert Einstein
On Sat, Oct 2, 2010 at 2:31 AM, Barry Warsaw <barry@python.org> wrote:
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
On Sep 30, 2010, at 10:47 AM, Jesse Noller wrote:
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
participants (27)
-
"Martin v. Löwis" -
Alexander Belopolsky -
Andi Albrecht -
Antoine Pitrou -
Barry Warsaw -
Benjamin Peterson -
Brett Cannon -
Brett Cannon -
Brian Curtin -
Daniel Stutzbach -
Dirkjan Ochtman -
exarkun＠twistedmatrix.com -
Fred Drake -
Georg Brandl -
geremy condra -
Guido van Rossum -
Jesse Noller -
Jesus Cea -
Laurens Van Houtven -
Martin Geisler -
Nick Coghlan -
Paul Moore -
Robert Kern -
Senthil Kumaran -
skip＠pobox.com -
Stephen J. Turnbull -
Toshio Kuratomi