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. - 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". - 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). 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). 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? 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? Cheers Andrea -- Andrea Aime OpenGeo - http://opengeo.org Expert service straight from the developers. ------------------------------------------------------------------------------ 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
