Open Stack

+1

Regards
-Harshad


> On Nov 6, 2013, at 12:36 AM, Radomir Dopieralski <openstack at 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 at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev