<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Following up on my previous email:<div class=""><br class=""></div><div class="">Every blueprint specification has a Work items section. That section should describe granular work items, not just things in general. We should encourage components leads to put their attention to this aspect.</div><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">7 груд. 2015 р. о 12:23 Roman Prykhodchenko <<a href="mailto:me@romcheg.me" class="">me@romcheg.me</a>> написав(ла):</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Maciej, thanks for bringing this topic up for the discussion!<div class=""><div class=""><br class=""></div><div class="">I absolutely agree with the idea that at this point we have a problem with patch size. Patches that are too big usually have two major issues: they either don’t get reviewed for a very long time or get random +1s from people who don’t even bother reading all the changes there.</div><div class=""><br class=""></div><div class="">Splitting a patch when it’s already published may be pretty problematic, so we should think about granularity of every feature on the design stage. Only in that way we will be able to submit small patches w/o making compromises on test coverage, stability or any other important aspect of the code quality.</div><div class=""><br class=""></div><div class="">Folks proposed two different solutions here: </div><div class=""> 1. Set a CI job that puts a -1 to every patch that’s bigger than a certain limit.</div><div class=""> 2. Make reviewers responsible for -1ing a patch.</div><div class=""><br class=""></div><div class="">Most of the patches that are bigger than 500 LOC can be split. However, there is a lot of corner cases we have to carry about, if we are going to put a -1 to all of them automatically. In my opinion we should not do that. Instead, we can send warning emails to core teams if this kind of patch set is submitted. Then a core team may easily analyze whether this patch can be split and propose a meaningful way to do that.</div><div class=""><br class=""></div><div class="">Huge patches cause a different story — they are incredibly hard for making a good review, merging or reverting them can lead to a major regression in a lot of places, they cause conflicts for a lot of other patches and so on. A huge patch ALWAYS means exclusively one of the following:</div><div class=""> 1. It can be split</div><div class=""> 2. The design of the feature is completely wrong.</div><div class=""><br class=""></div><div class="">We need to set a treshold for a huge patch and block all patches that exceed it by putting -2 scores.</div><div class=""><br class=""></div><div class="">Summary:</div><div class=""><br class=""></div><div class="">- Let’s set two thresholds — one for to big and one for huge patches.</div><div class="">- Let’s create a job that will warn an appropriate core team if a too big patch is submitted and block huge patches.</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">- romcheg</div><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><br class=""><div class=""><div class=""><div class=""><blockquote type="cite" class=""><div class="">7 груд. 2015 р. о 11:23 Stanislaw Bogatkin <<a href="mailto:sbogatkin@mirantis.com" class="">sbogatkin@mirantis.com</a>> написав(ла):</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">> What if you're not sure how the improved code should look like, but<br class="">> you definitely sure that it shouldn't look like proposed one? :)<div class=""><br class=""></div><div class="">I believe you should ask other people if you are right, as set '-1' to code that you</div><div class="">cannot improve is not the best option, so</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">> If you are not sure how the improved code would look like, just let it go!<br class=""></div><div class="">is right</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">Also I think that set some strict boundaries, like 400 LOC per patch is wrong. For example, if you</div><div class="">introduce new test fixture for fuel-library, it usually about 800+ LOC and you can't split it</div><div class="">out, so we should either move such fixtures out of code or make an exeption for such type of code.<br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Mon, Dec 7, 2015 at 1:03 PM, Igor Kalnitsky <span dir="ltr" class=""><<a href="mailto:ikalnitsky@mirantis.com" target="_blank" class="">ikalnitsky@mirantis.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hey Andrii,<br class="">
<br class="">
I'm totally agree with you about contribution guides, except one thing<br class="">
- we need and should force users to split huge patches into set of<br class="">
small ones. Mostly because it will simplify support and review of<br class="">
patches. Previously we ignored this rule pretty often, and get a lot<br class="">
of troubles due to buggy code.<br class="">
<br class="">
Also, see some comments below.<br class="">
<span class=""><br class="">
> So, when an author of a patch gets -1 with the statement «split this<br class="">
> code», I believe it is not constructive. At least you should roughly<br class="">
> describe how you see it, how the patch could be split, you should be<br class="">
> helpful to the author of a patch.<br class="">
<br class="">
</span>No one is suggesting to set -1 without leaving a comment how the patch<br class="">
could be divided.<br class="">
<span class=""><br class="">
<br class="">
> If you are not sure how the improved code would look like, just let it go!<br class="">
<br class="">
</span>What if you're not sure how the improved code should look like, but<br class="">
you definitely sure that it shouldn't look like proposed one? :)<br class="">
<br class="">
I'd say it's a good reason to set -1 and start discussion. Not only<br class="">
with author, but with other folks, since everyone in community is<br class="">
responsible of code quality of the project.<br class="">
<span class=""><font color="#888888" class=""><br class="">
- I<br class="">
</font></span><div class=""><div class="h5"><br class="">
On Mon, Dec 7, 2015 at 3:28 AM, Andrey Tykhonov <<a href="mailto:atykhonov@mirantis.com" class="">atykhonov@mirantis.com</a>> wrote:<br class="">
> I believe this is against the code review guidelines.<br class="">
><br class="">
> «Comments must be meaningful and should help an author to change the<br class="">
> code the right way.» [1]<br class="">
><br class="">
> If you get a comment that says «split this change into the smaller<br class="">
> commit» I'm sorry, but it doesn't help at all.<br class="">
><br class="">
> «Leave constructive comments<br class="">
><br class="">
> Not everyone in the community is a native English speaker, so make<br class="">
> sure your remarks are meaningful and helpful for the patch author to<br class="">
> change his code, but also polite and respectful.<br class="">
><br class="">
> The review is not really about the score. It's all about the<br class="">
> comments. When you are reviewing code, always make sure that your<br class="">
> comments are useful and helpful to the author of the patch. Try to<br class="">
> avoid leaving comments just to show that you reviewed something if<br class="">
> they don't really add anything meaningful» [2]<br class="">
><br class="">
> So, when an author of a patch gets -1 with the statement «split this<br class="">
> code», I believe it is not constructive. At least you should roughly<br class="">
> describe how you see it, how the patch could be split, you should be<br class="">
> helpful to the author of a patch. So, first of all, you need to review<br class="">
> the patch! :)<br class="">
><br class="">
> I want to emphasize this: «The review is not really about the<br class="">
> score. It's all about the comments.»<br class="">
><br class="">
> «In almost all cases, a negative review should be accompanied by clear<br class="">
> instructions for the submitter how they might fix the patch.» [4]<br class="">
><br class="">
> I believe that the statement "split this change into the smaller<br class="">
> commit" is too generic, it is mostly the same as the "this patch needs<br class="">
> further work". It doesn't bring any additional instructions how<br class="">
> exactly a patch could be fixed.<br class="">
><br class="">
> Please also take a loot at the following conversation from mailing<br class="">
> list: [3].<br class="">
><br class="">
> «It's not so easy to guess what you mean, and in case of a clumsy<br class="">
> piece of code, not even that certain that better code can be used<br class="">
> instead. So always provide an example of what you would rather want to<br class="">
> see. So instead of pointing to indentation rules, just show properly<br class="">
> indented code. Instead of talking about grammar or spelling, just type<br class="">
> the corrected comment or docstring. Finally, instead of saying "use<br class="">
> list comprehension here" or "don't use has_key", just type your<br class="">
> proposal of how the code should look like. Then we have something<br class="">
> concrete to talk about. Of course, you can also say why you think this<br class="">
> is better, but an example is very important. If you are not sure how<br class="">
> the improved code would look like, just let it go, chances are it<br class="">
> would look even worse.» [3]<br class="">
><br class="">
> So, please, bring something concrete to talk about. If you are not<br class="">
> sure how the improved code would look like, just let it go!<br class="">
><br class="">
> «The simplest way to talk about code is to just show the code. When you<br class="">
> want the author to fix something, rewrite it in a different way,<br class="">
> format the code differently, etc. -- it's best to just write in the<br class="">
> comment how you want that code to look like. It's much faster than<br class="">
> having the author guess what you meant in your descriptions, and also<br class="">
> lets us learn much faster by seeing examples.» [2]<br class="">
><br class="">
><br class="">
> [1]<br class="">
> <a href="https://docs.google.com/document/d/1tyKhHQRQqTEW6tS7_LCajEpzqn55f-f5nDmtzIeJ2uY/edit" rel="noreferrer" target="_blank" class="">https://docs.google.com/document/d/1tyKhHQRQqTEW6tS7_LCajEpzqn55f-f5nDmtzIeJ2uY/edit</a><br class="">
> [2] <a href="https://wiki.openstack.org/wiki/CodeReviewGuidelines" rel="noreferrer" target="_blank" class="">https://wiki.openstack.org/wiki/CodeReviewGuidelines</a><br class="">
> [3]<br class="">
> <a href="http://www.mail-archive.com/openstack-dev@lists.openstack.org/msg07831.html" rel="noreferrer" target="_blank" class="">http://www.mail-archive.com/openstack-dev@lists.openstack.org/msg07831.html</a><br class="">
> [4] <a href="http://docs.openstack.org/infra/manual/developers.html#peer-review" rel="noreferrer" target="_blank" class="">http://docs.openstack.org/infra/manual/developers.html#peer-review</a><br class="">
><br class="">
><br class="">
> Best regards,<br class="">
> Andrey Tykhonov (tkhno)<br class="">
><br class="">
><br class="">
</div></div><div class=""><div class="h5">> __________________________________________________________________________<br class="">
> OpenStack Development Mailing List (not for usage questions)<br class="">
> Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org/?subject:unsubscribe" rel="noreferrer" target="_blank" class="">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br class="">
> <a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank" class="">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br class="">
><br class="">
<br class="">
__________________________________________________________________________<br class="">
OpenStack Development Mailing List (not for usage questions)<br class="">
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org/?subject:unsubscribe" rel="noreferrer" target="_blank" class="">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br class="">
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank" class="">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br class="">
</div></div></blockquote></div><br class=""></div></div></div>
__________________________________________________________________________<br class="">OpenStack Development Mailing List (not for usage questions)<br class="">Unsubscribe: <a href="mailto:OpenStack-dev-request@lists.openstack.org" class="">OpenStack-dev-request@lists.openstack.org</a>?subject:unsubscribe<br class=""><a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" class="">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br class=""></div></blockquote></div><br class=""></div></div></div></div></div></div></blockquote></div><br class=""></div></body></html>