+1 Regards -Harshad
> On Nov 6, 2013, at 12:36 AM, Radomir Dopieralski <openst...@sheep.art.pl> > wrote: > > Hello, > > I'm quite new in the OpenStack project, but I love it already. What is > especially nifty is the automated review system -- I'm really impressed. > I'm coming from a project in which we also did reviews of every change > -- although it was mostly manual, and just one review was enough to > merge -- and at some point in that project I noticed that it is very > easy to give reviews that are unhelpful, frustrating and just get in the > way of the actual work. I started paying attention to how I am reviewing > code, and I managed to come up with several patterns that are bad. Once > I know the patterns, it's easier to recognize when I'm doing something > wrong and rethink the review. I would like to share the patterns that I > noticed. > > > Not sure if that works... > ========================= > > You see some suspicious piece of code, and you are not sure if it is > correct or not. So you point it out in the review and -1 it, demanding > that the author rechecks it and/or prove that it indeed works. > > It's your job as a reviewer to check such things, don't put all the > effort on the author. They probably already checked that this code > works, and possibly have even written tests for it. If you are not > familiar with the technology enough to tell whether it's good or not, > and have no means of testig it yourself, you shouldn't be reviewing it. > If you do have the means to test it or can find the piece of > documentation that says that it shouldn't be done -- do it as a part of > the review. > > > You ain't gonna need it. > ======================== > > The code contains some parts that are potentially reusable later or in > different areas, so you -1 it and tell the author to move them into > reusable functions. > > The fact that you think something is reusable doesn't necessarily mean > it is, and overly general code is harder to maintain. Let something > repeat a couple of times just to be sure it actually is reusable. Once > you find a repeating pattern, you can refactor the code to use a > generalized function in its place -- in a separate change. > > > There is an old bug here. > ========================= > > While you review the submitted code, you notice something wrong in the > code not immediately related to the change you are reviewing. You -1 the > change and tell the author to fix that bug, or formatting issue, or > typo, or whatever. > > That fix has nothing to do with the change you are reviewing. The > correct thing to do is to make a mental note of it, and fix it in a > separate change -- possibly even finding more instances of it or coming > up with a much better fix after seeing more code. > > > Guess what I mean. > ================== > > You notice a pep8 violation, or pep257 violation, or awkward wording of > a comment or docstring, or a clumsy piece of code. You -1 the change and > just tell author to "fix it". > > It's not so easy to guess what you mean, and in case of a clumsy piece > of code, not even that certain that better code can be used instead. So > always provide an example of what you would rather want to see. So > instead of pointing to indentation rules, just show properly indented > code. Instead of talking about grammar or spelling, just type the > corrected comment or docstring. Finally, instead of saying "use list > comprehension here" or "don't use has_key", just type your proposal of > how the code should look like. Then we have something concrete to talk > about. Of course, you can also say why you think this is better, but an > example is very important. If you are not sure how the improved code > would look like, just let it go, chances are it would look even worse. > > > Not a complete fix. > =================== > > The code fixes some problems (for example, fixes formatting and enables > some flake8 checks), but leaves some other, related problems still not > fixed. You -1 it and demand that the author adds fixes for the related > problem. > > Don't live your coding career through the authors. If their fix is good > and correct and improves the code, let it in, even if you see more > opportunities to improve it. You can add those additional fixes yourself > in a separate change. Or, if you don't have time or skill to do that, > report a bug about the remaining problems and someone else will do it, > but don't hold the author hostage with your review because you think he > didn't do enough. > > > Leaving a mark. > =============== > > You review a change and see that it is mostly fine, but you feel that > since you did so much work reviewing it, you should at least find > *something* wrong. So you find some nitpick and -1 the change just so > that they know you reviewed it. > > This is quite obvious. Just don't do it. It's OK to spend an hour > reviewing something, and then leaving no comments on it, because it's > simply fine, or because we had to means to test someting (see the first > pattern). > > > > Those are the things I'm careful about myself. I'm sure that not > everyone of you will agree that all of those patterns are bad in all > situations -- in fact, I'm pretty sure that some of them are sometimes > warranted -- but they are always suspicious, and when I find myself > falling into one of them, I always rethink what I'm doing. Maybe you > have some more bad patterns that you would like to share? Reviewing code > is a difficult skill and it's always good to improve it by using > experience of others. > > -- > Radomir Dopieralski > > _______________________________________________ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev