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.
