<div dir="ltr">Hi,<div class="gmail_extra"><div><div class="gmail_signature"><div dir="ltr"><br></div></div></div><div class="gmail_quote">On Mon, Dec 7, 2015 at 2:28 AM, Andrey Tykhonov <span dir="ltr"><<a href="mailto:atykhonov@mirantis.com" target="_blank">atykhonov@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I believe this is against the code review guidelines.<br><br>«Comments must be meaningful and should help an author to change the<br>code the right way.» [1]<br></div></blockquote><div><br></div><div>Could you provide a link that accessible by community? Thanks a lot in advance.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br>If you get a comment that says «split this change into the smaller<br>commit» I'm sorry, but it doesn't help at all.<br><br>«Leave constructive comments<br><br>Not everyone in the community is a native English speaker, so make<br>sure your remarks are meaningful and helpful for the patch author to<br>change his code, but <u>also polite and respectful</u>.<br><br>The review is not really about the score. It's all about the<br>comments. When you are reviewing code, always make sure that your<br>comments are useful and helpful to the author of the patch. Try to<br>avoid leaving comments just to show that you reviewed something if<br>they don't really add anything meaningful» [2]<br><br>So, when an author of a patch gets -1 with the statement «split this<br>code», I believe it is not constructive. At least you should roughly<br>describe how you see it, how the patch could be split, you should be<br>helpful to the author of a patch. So, first of all, you need to review<br>the patch! :)<br><br>I want to emphasize this: «<u><b>The review is not really about the<br>score. It's all about the comments.</b></u>»<br><br>«In almost all cases, a negative review should be accompanied by <b>clear<br>instructions</b> for the submitter how they might fix the patch.» [4]<br><br>I believe that the statement "split this change into the smaller<br>commit" is too generic, it is mostly the same as the "this patch needs<br>further work". It doesn't bring any additional instructions how<br>exactly a patch could be fixed.<br><br>Please also take a loot at the following conversation from mailing<br>list: [3].<br><br>«It's not so easy to guess what you mean, and in case of a clumsy<br>piece of code, not even that certain that better code can be used<br>instead. So always provide an example of what you would rather want to<br>see. So instead of pointing to indentation rules, just show properly<br>indented code. Instead of talking about grammar or spelling, just type<br>the corrected comment or docstring. Finally, instead of saying "use<br>list comprehension here" or "don't use has_key", just type your<br>proposal of how the code should look like. Then we have something<br>concrete to talk about. Of course, you can also say why you think this<br>is better, but an example is very important. If you are not sure how<br>the improved code would look like, just let it go, chances are it<br>would look even worse.» [3]<br><br>So, please, bring something concrete to talk about. If you are not<br>sure how the improved code would look like, just let it go!<br><br>«The simplest way to talk about code is to just show the code. When you<br>want the author to fix something, rewrite it in a different way,<br>format the code differently, etc. -- it's best to just write in the<br>comment how you want that code to look like. It's much faster than<br>having the author guess what you meant in your descriptions, and also<br>lets us learn much faster by seeing examples.» [2]<br><br><br>[1] <a href="https://docs.google.com/document/d/1tyKhHQRQqTEW6tS7_LCajEpzqn55f-f5nDmtzIeJ2uY/edit" target="_blank">https://docs.google.com/document/d/1tyKhHQRQqTEW6tS7_LCajEpzqn55f-f5nDmtzIeJ2uY/edit</a><br>[2] <a href="https://wiki.openstack.org/wiki/CodeReviewGuidelines" target="_blank">https://wiki.openstack.org/wiki/CodeReviewGuidelines</a><br>[3] <a href="http://www.mail-archive.com/openstack-dev@lists.openstack.org/msg07831.html" target="_blank">http://www.mail-archive.com/openstack-dev@lists.openstack.org/msg07831.html</a><br>[4] <a href="http://docs.openstack.org/infra/manual/developers.html#peer-review" target="_blank">http://docs.openstack.org/infra/manual/developers.html#peer-review</a><br><br><br>Best regards,<br>Andrey Tykhonov (tkhno)<br><br></div>
<br>__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
<br></blockquote></div><br></div></div>