Open Stack

I believe this is against the code review guidelines.

«Comments must be meaningful and should help an author to change the
code the right way.» [1]

If you get a comment that says «split this change into the smaller
commit» I'm sorry, but it doesn't help at all.

«Leave constructive comments

Not everyone in the community is a native English speaker, so make
sure your remarks are meaningful and helpful for the patch author to
change his code, but *also polite and respectful*.

The review is not really about the score. It's all about the
comments. When you are reviewing code, always make sure that your
comments are useful and helpful to the author of the patch. Try to
avoid leaving comments just to show that you reviewed something if
they don't really add anything meaningful» [2]

So, when an author of a patch gets -1 with the statement «split this
code», I believe it is not constructive. At least you should roughly
describe how you see it, how the patch could be split, you should be
helpful to the author of a patch. So, first of all, you need to review
the patch! :)

I want to emphasize this: «
*The review is not really about thescore. It's all about the comments.*»

«In almost all cases, a negative review should be accompanied by
*clearinstructions* for the submitter how they might fix the patch.» [4]

I believe that the statement "split this change into the smaller
commit" is too generic, it is mostly the same as the "this patch needs
further work". It doesn't bring any additional instructions how
exactly a patch could be fixed.

Please also take a loot at the following conversation from mailing
list: [3].

«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.» [3]

So, please, bring something concrete to talk about. If you are not
sure how the improved code would look like, just let it go!

«The simplest way to talk about code is to just show the code. When you
want the author to fix something, rewrite it in a different way,
format the code differently, etc. -- it's best to just write in the
comment how you want that code to look like. It's much faster than
having the author guess what you meant in your descriptions, and also
lets us learn much faster by seeing examples.» [2]


[1]
https://docs.google.com/document/d/1tyKhHQRQqTEW6tS7_LCajEpzqn55f-f5nDmtzIeJ2uY/edit
[2] https://wiki.openstack.org/wiki/CodeReviewGuidelines
[3]
http://www.mail-archive.com/openstack-dev@lists.openstack.org/msg07831.html
[4] http://docs.openstack.org/infra/manual/developers.html#peer-review


Best regards,
Andrey Tykhonov (tkhno)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20151207/70b8d220/attachment.html>