<div dir="ltr">Hey Igor! Thank you for the response!<br><div><div class="gmail_extra"><br><div class="gmail_quote">On 7 December 2015 at 12:03, Igor Kalnitsky <span dir="ltr"><<a href="mailto:ikalnitsky@mirantis.com" target="_blank">ikalnitsky@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hey Andrii,<br>
<br>
I'm totally agree with you about contribution guides, except one thing<br>
- we need and should force users to split huge patches into set of<br>
small ones.<br></blockquote><div><br>I believe that this is much more productive and efficient when team<br>members are helpful than forceful to each other :) I just want to<br>say, that I would wish to have on a project as the first, most<br>important, and with the highest priority, rule: be helpful :)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Mostly because it will simplify support and review of<br>
patches. Previously we ignored this rule pretty often, and get a lot<br>
of troubles due to buggy code.<br></blockquote><div><br>It seems that an author of a patch and a reviewer are on the same side<br>on this matter :) Definitely it is not easy (for an author) to<br>support, write a huge amount of unit tests, to fix comments, and<br>overall to develop a large patch. I know this is not the best way. It<br>is not easy on all stages of patch existence. But if it happens, let's<br>try to make it better together. Because we are on the same side on<br>this matter, because it is not easy for all of us, not just for<br>reviewers.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Also, see some comments below.<br>
<span><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.<br>
<br>
</span>No one is suggesting to set -1 without leaving a comment how the patch<br>
could be divided.<br></blockquote><div><br>Yes, I know. But for sorry, this is how it did work for me (in the<br>case of the large patch). And, having additional rule does not make me<br>thinking that something could be improved. We have already excellent<br>code review guidelines. Why we need something more, I really don't<br>understand.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span><br>
<br>
> If you are not sure how the improved code would look like, just let it go!<br>
<br>
</span>What if you're not sure how the improved code should look like, but<br>
you definitely sure that it shouldn't look like proposed one? :)<br></blockquote><div><br></div><div>Yes, of course, it could be not easy case.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I'd say it's a good reason to set -1 and start discussion. Not only<br>
with author, but with other folks, since everyone in community is<br>
responsible of code quality of the project.<br></blockquote><div><br></div><div>Yes, agree. Good discussion helps a lot.<br><br><br><br></div><div>Thank you!<br><br><br></div><div>Andrey<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- I<br>
<div><div><br>
On Mon, Dec 7, 2015 at 3:28 AM, Andrey Tykhonov <<a href="mailto:atykhonov@mirantis.com" target="_blank">atykhonov@mirantis.com</a>> wrote:<br>
> 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>
><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 also polite and respectful.<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: «The review is not really about the<br>
> score. It's all about the comments.»<br>
><br>
> «In almost all cases, a negative review should be accompanied by clear<br>
> instructions 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]<br>
> <a href="https://docs.google.com/document/d/1tyKhHQRQqTEW6tS7_LCajEpzqn55f-f5nDmtzIeJ2uY/edit" rel="noreferrer" target="_blank">https://docs.google.com/document/d/1tyKhHQRQqTEW6tS7_LCajEpzqn55f-f5nDmtzIeJ2uY/edit</a><br>
> [2] <a href="https://wiki.openstack.org/wiki/CodeReviewGuidelines" rel="noreferrer" target="_blank">https://wiki.openstack.org/wiki/CodeReviewGuidelines</a><br>
> [3]<br>
> <a href="http://www.mail-archive.com/openstack-dev@lists.openstack.org/msg07831.html" rel="noreferrer" 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" rel="noreferrer" target="_blank">http://docs.openstack.org/infra/manual/developers.html#peer-review</a><br>
><br>
><br>
> Best regards,<br>
> Andrey Tykhonov (tkhno)<br>
><br>
><br>
</div></div>> __________________________________________________________________________<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>
<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>
</blockquote></div><br></div></div></div>