[openstack-dev] [Fuel] Patch size limit

Sergii Golovatiuk sgolovatiuk at mirantis.com
Mon Dec 7 10:20:48 UTC 2015


Hi,

On Mon, Dec 7, 2015 at 2:28 AM, Andrey Tykhonov <atykhonov at mirantis.com>
wrote:

> 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]
>

Could you provide a link that accessible by community? Thanks a lot in
advance.


>
> 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)
>
>
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20151207/1d4cf592/attachment.html>


More information about the OpenStack-dev mailing list