One problem with attaching the patches is code context. The patch only lists the lines that change, not all the code around where the change occurred. It is very easy to miss the bigger picture if all you see is a patch. This becomes especially important with larger submissions like UV's palette quantization RFC. The only way to really review something like this is to apply the patch yourself and do the diffs. That's a very time consuming process for the reviewer, assuming they even have disk space available on their workstation.
Larger submissions are easier to handle with online code review tools. At some point, we should evaluate code review tools. I'm sure all the Autodesk developers (and ex developers like myself) would be very happy if the MapGuide Open Source project adopted Smart Bear's Code Collaborator
http://smartbear.com/codecollab.php. It is free for open source projects.
However, the server requirements seem a little steep...
Server Requirements: At least 3.5GHz processor, 2GB of RAM, 10,000 RPM hard drives (preferably SCSI) and 100GB of hard drive space on a dedicated server. Database may be installed on the same machine as the web server or requires high speed connection (at least 100mbps LAN).
I have enough virtualized capacity to provide a 2GHz processor, 2GB of ram, and 40GB of disk to a code review VM. With a virtualized environment, these numbers could increase over time. However, my basement is not a fully redundant data center so I cannot guarantee uptime. That might be a problem for Autodesk since the team is geographically disperse.
We could also look at renting a dedicated server but prices start at $175/month USD for similar specs. Virtualized hosting is similar. Alternatively, we could move a portion of my rack to a data center for somewhere around $500/month. That would give us more flexibility but I will have to verify prices.
Thanks,
Trevor
-----Original Message-----
From:
[hidden email] [mailto:
[hidden email]] On Behalf Of Tom Fukushima
Sent: July 30, 2009 1:54 PM
To: MapGuide Internals Mail List
Subject: RE: [mapguide-internals] Please review patch for ticket #1057
Note that trac is pretty good, and if you click on the patch link below, it opens up a page that also has a link to the ticket; so it's not much of an issue. However, that said, I agree with Zac, and think that a link to the ticket is better. Details about which patches should be reviewed (in case there are multiple) could be in the ticket details.
-----Original Message-----
From:
[hidden email] [mailto:
[hidden email]] On Behalf Of Zac Spitzer
Sent: Thursday, July 30, 2009 9:13 AM
To: MapGuide Internals Mail List
Subject: Re: [mapguide-internals] Please review patch for ticket #1057
linking to the ticket is a lot easier to review
https://trac.osgeo.org/mapguide/ticket/1057On Thu, Jul 30, 2009 at 6:28 PM, Christine
Bao<
[hidden email]> wrote:
--
Zac Spitzer -
http://zacster.blogspot.com+61 405 847 168
_______________________________________________
mapguide-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/mapguide-internals_______________________________________________
mapguide-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/mapguide-internals_______________________________________________
mapguide-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/mapguide-internals
