I like the sound of this, I think this is the right level for the review process. Perhaps in item 2, we might say ³when the developer feels enough has been done to be reviewed, but not more than 200 lines of code...²
I missed the ³patch² process, but if there is one, that is good, and hopefully would avoid the whole 8-step process for something trivial. From: "Andrew Connolly" <[EMAIL PROTECTED]> Reply-To: [EMAIL PROTECTED], LSST Data Management <[email protected]> Date: Wed, 29 Nov 2006 09:39:23 -0500 To: "LSST Data Management" <[email protected]> Subject: Re: [LSST-data] DataChallenge: Thoughts on software process using trac I looked through the write up and it seems to touch on most of the areas we discussed in Tucson about how to instigate a code review process for the development. I think in fact it goes a little overboard in terms of the burden it puts on the reviewer (ie merge the branch, compile, run unit tests, make sure it wont break the main trunk). Making sure something merges and that unit test run etc should be the coders responsibility and the reviewer is supposed to be providing a second set of eyes on the code to look for any obvious issues. The procedure that I was envisaging was a bit simpler: 1. A task is assigned to some one to complete (something that is small - ie not write the framework) 2. When about 100-200 lines of code are written the coder requests a review (before or when checking in) 3. The request is emailed to a person to review (the coder chooses the person - someone in the development group who understands the area of code the coder is working on) 4. The reviewer is presented with the original code, the changed code with the lines changes/added/deleted highlighted (using something like kdiff3 or winmerge) 5. The reviewers responsibility is to review the code for obvious errors, check that it is readable, check that unit tests exist, check it conforms to the coding practices and that the algorithm is reasonable. The reviewer is encouraged to apply the changes and run the unit tests etc but this is not required. 6. The reviewer, either on a wiki or via email to a central repository, approves/rejects the changes with comments and they are checked in (or checked off) 7. Reviews should be done within 24 hrs 8. If we allow review after checkin we log on a wiki which pieces of code have been reviewed and a manager makes sure that this doesnt fall behind I think that this fits within the UltimateQualityDevelopmentSystem <http://divmod.org/trac/wiki/UltimateQualityDevelopmentSystem> process but makes a review a relatively quick process. I assume that the question of whether to use branches for the assigned tasks is because svn doesnt run in a changelist manner? If the code review process is done every few hundred lines this might not be such a big deal. I think for small changes UQDS suggested a patch system on top of the standard code review anyway. cheers Andy On 11/28/06, Jeffrey P Kantor <[EMAIL PROTECTED]> wrote: > Hi Robert, > > The process in general sounds like something close to what we want, although > I am a bit uneasy about opening a new branch for every single modification > (unless I missed the authors intent). I think we might want to moderate > this a bit, especially while our "specs" are undergoing development, or for > fixes to relatively obvious errors. For example, do we really want an open > branch, check out, modify, test, check in, review, merge branch sequence > because someone renamed a variable to meet a coding standard, or adjusted a > counter by 1 to fix a logical error? > > Jeff > >> > From: Robert Lupton <[EMAIL PROTECTED] >> <mailto:[EMAIL PROTECTED]> > >> > Reply-To: LSST Data Management <[email protected]> >> > Date: Tue, 21 Nov 2006 15:39:22 -0500 >> > To: LSST Data Management < [email protected] >> <mailto:[email protected]> > >> > Subject: [LSST-data] DataChallenge: Thoughts on software process using trac >> > >> > Pursuant to my mail about using trac, Craig Loomis >> > ( [EMAIL PROTECTED] <mailto:[EMAIL PROTECTED]> ) >> > commented: >> > >>> >> If you are thinking in this vein, I'll commend a link I recently >>> >> bumped into >>> >> describing a (stupidly named) development cycle. It feels _doable_ >>> >> on a >>> >> personal/human level, mainly because of its essential clarity. And >>> >> I can >>> >> think of many code/management/human pathologies which would it >>> >> would help avert. >>> >> http://divmod.org/trac/wiki/UltimateQualityDevelopmentSystem. >> > >> > I found this a worthwhile read. We've talked about code reviews, and >> > this >> > is an interesting way of approaching the problem. >> > >> > R >> > >> > _______________________________________________ >> > LSST-data mailing list >> > [email protected] >> > http://www.lsstmail.org/mailman/listinfo/lsst-data >> > > > _______________________________________________ > LSST-data mailing list > [email protected] > http://www.lsstmail.org/mailman/listinfo/lsst-data _______________________________________________ LSST-data mailing list [email protected] http://www.lsstmail.org/mailman/listinfo/lsst-data
_______________________________________________ LSST-data mailing list [email protected] http://www.lsstmail.org/mailman/listinfo/lsst-data
