All great points Andrea. Some comments inline. Andrea Aime wrote: > Justin Deoliveira ha scritto: >> Hi all, >> >> Lately there has been a lot of talk around review practices. And in >> general review is something that has becoming more frequent (which I >> think is a great thing). > > I guess one background question that we need to agree upon is: > do we want to raise the code quality and is software reviews the > way to get there? > I'm +1 on this, but given reviews take work, we need the commitment > of everybody else too. > >> However, without an official policy in place review has been ambiguous, >> with some people wondering when they need a review, etc... >> >> So in an effort to rectify this I would like to come up with a code >> review policy in GeoServer. I have put together a wiki page to foster >> discussion: >> >> http://geoserver.org/display/GEOS/Review+Practices >> >> It needs to be expanded in a couple of places. After everyone weighs in >> we can put together the official GSIP and update the developer guide. > > Some opinions: > - I would say, let's just mandate pre-commit review on the bigger stuff. > No post commit review as a rule, thought people is invited to keep > an eye on what other developers are doing and try to be helpful. Just to state my preference I would prefer pre-comit review as well. > - who reviews what... stingy one. > I'd say let's nominate module maintainers, but make sure we have at > least two official maintainers per core module (if we get more, > even better) so that they can review each other. > The maintainer concept should be not as strong as in GeoTools imho, > it should be more of a reviewing responsibility than "this is my > module and I do whatever I please with it". Perhaps we should just call this role "reviewer", rather than "maintainer" to make it more explicit. > - what makes for big enough patches? Ha :-) > I'd say every new feature is big enough, no matter how small (that is, > the quality of the patch makes it review worthy, not much its size) > For bug fixes hmmm.... there are small ones and bigger ones for sure. > Some may be 10 lines with a patch outside of main/ows/platform, I > would not feel like asking review for those (small enough to be > understandable by looking at the commit list I'd say). > Some require reworking a group of classes, or it's code that is > used in a lot of places (stuff in main/ows), a review would be > advisable. Especially in main/ows changes a look at the patch > from whoever maintains affected modules is advisable (but > not mandatory). I agree here, but it is still a blurry line as to what needs to be reviewed and what does not. I think I would push for any change to a core module has to be reviewed, unless undeniably trivial. And in non-core modules we trust the person applying the patch to make the call, trying to provide as many guidelines as we can.
> > One thing that worries me about reviews is developer getting > stuck waiting for a review, be frustrated, move to something else. > We have to avoid that at all costs imho. > I would like to see the following: > - when there is a need for a review it should become the > highest priority for the other developers. At least, saying > "I'll review this" should be something that pops up within > the day (having somone that declares he'll do the review should > happen quickly) > - the review itself should be taken care of quickly. No more > than a week delay imho unless the patch is really huge. > What I mean is that the review system should flow like clear > water, if developers start feeling stuck like in quicksands > we have a problem (a serious one). Agreed, there should be some time limit on a review. Ideally something in proportion to the size of the patch. I mean... the initial app-schema patch I reviewed was huge, doing it on one week would have been a bit tough, especially since it involved actually applying and doing testing. I know this is a very special case, but still putting a hard number leaves the back door open. One week also may not be ideal in cases where a developer has to take a leave of some sort, and is the primary reviewer. What about this: We call the time limit one week to review, unless: a) the patch is too big to review in a week b) the reviewer has asked for and obtained more time from the submitter for the review Now, let's say after a week nothing has been reviewed. I would say the original submitter can apply the patch without review, *but*, posts the patch for post-commit review in crucible or whatever tool we adopt. This way the developer is not held up, but the code still has a chance to be reviewed. If no review happens, it is the fault of the reviewer. > > Another very important thing is that we should have guidelines > on how a review should be approached. Nothing big, just > a good read to avoid reviews becoming dogfights over minor > points. For example this one seems helpful: > http://www.developer.com/tech/article.php/3579756 > > Finally the process needs to be filled in. How does one > post a patch for review? How does he ask for review? > Do we use a tool like crucible or just attach a patch to > jira? Yeah, I was thinking that getting everyones preferences would be good before starting to work on the actual step by step, since depending on how people want to go it will affect how the process works. > > Code quality points should be detailed better. Looking at the > existing ones performance can be measured, looking for edge > cases and proper error handling is something that can be done > via a line by line review, consistency can be checked by looking > at other classes in the GeoServer code base. > Aestetics wise heh, I don't know what is to be expected. > Respecting the code conventions is an easy aestetics point. > Having a checklist of others would be nice (like the style > guide in the documentation. > Maybe we could add that the changed code should pass a findbugs > examination? Since I am the usually the one commenting on such things I can come up with a "consistency" check list. But it usually (believe it or not) just comes down to names of packages, classes, methods, etc... making sure when coming up with a new name for something, you look at the names already there, and come up with a consistent one. > > Cheers > Andrea > -- Justin Deoliveira OpenGeo - http://opengeo.org Enterprise support for open source geospatial. ------------------------------------------------------------------------------ Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day trial. Simplify your report design, integration and deployment - and focus on what you do best, core application coding. Discover what's new with Crystal Reports now. http://p.sf.net/sfu/bobj-july _______________________________________________ Geoserver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geoserver-devel
