Hi, On Mon, Dec 7, 2015 at 2:28 AM, Andrey Tykhonov <[email protected]> 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/[email protected]/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: [email protected]?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > >
__________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: [email protected]?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
