Open Stack

Hey Igor! Thank you for the response!

On 7 December 2015 at 12:03, Igor Kalnitsky <ikalnitsky at mirantis.com> wrote:

> Hey Andrii,
>
> I'm totally agree with you about contribution guides, except one thing
> - we need and should force users to split huge patches into set of
> small ones.
>

I believe that this is much more productive and efficient when team
members are helpful than forceful to each other :) I just want to
say, that I would wish to have on a project as the first, most
important, and with the highest priority, rule: be helpful :)


> Mostly because it will simplify support and review of
> patches. Previously we ignored this rule pretty often, and get a lot
> of troubles due to buggy code.
>

It seems that an author of a patch and a reviewer are on the same side
on this matter :) Definitely it is not easy (for an author) to
support, write a huge amount of unit tests, to fix comments, and
overall to develop a large patch. I know this is not the best way. It
is not easy on all stages of patch existence. But if it happens, let's
try to make it better together. Because we are on the same side on
this matter, because it is not easy for all of us, not just for
reviewers.


>
> Also, see some comments below.
>
> > 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.
>
> No one is suggesting to set -1 without leaving a comment how the patch
> could be divided.
>

Yes, I know. But for sorry, this is how it did work for me (in the
case of the large patch). And, having additional rule does not make me
thinking that something could be improved. We have already excellent
code review guidelines. Why we need something more, I really don't
understand.


>
>
> > If you are not sure how the improved code would look like, just let it
> go!
>
> What if you're not sure how the improved code should look like, but
> you definitely sure that it shouldn't look like proposed one? :)
>

Yes, of course, it could be not easy case.


>
> I'd say it's a good reason to set -1 and start discussion. Not only
> with author, but with other folks, since everyone in community is
> responsible of code quality of the project.
>

Yes, agree. Good discussion helps a lot.



Thank you!


Andrey



>
> - I
>
> On Mon, Dec 7, 2015 at 3: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]
> >
> > 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 the
> > score. It's all about the comments.»
> >
> > «In almost all cases, a negative review should be accompanied by clear
> > instructions 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
> >
>
> __________________________________________________________________________
> 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/20151208/a4642036/attachment.html>