Re: [openstack-dev] [Fuel] Patch size limit

[Andrey Tykhonov]

Hey Igor! Thank you for the response!

On 7 December 2015 at 12:03, Igor Kalnitsky  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 
> 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 

Re: [openstack-dev] [Fuel] Patch size limit

[Igor Kalnitsky]

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

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.


> 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? :)

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.

- I

On Mon, Dec 7, 2015 at 3:28 AM, Andrey Tykhonov  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-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>

__
OpenStack Development Mailing List (not for usage questions)

Re: [openstack-dev] [Fuel] Patch size limit

[Andrey Tykhonov]

On 7 December 2015 at 12:20, Sergii Golovatiuk 
wrote:

> Hi,
>
> On Mon, Dec 7, 2015 at 2:28 AM, Andrey Tykhonov 
> 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.
>


Sure! I'm sorry for this link.

BTW, if you even aren't able to open this link, you don't miss
anything because mostly the same is described in the code review
guidelines.


Thank you!


>
>
>>
>> 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-requ...@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-requ...@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-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Fuel] Patch size limit

[Stanislaw Bogatkin]

> 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? :)

I believe you should ask other people if you are right, as set '-1' to code
that you
cannot improve is not the best option, so


> If you are not sure how the improved code would look like, just let it go!
is right


Also I think that set some strict boundaries, like 400 LOC per patch is
wrong. For example, if you
introduce new test fixture for fuel-library, it usually about 800+ LOC and
you can't split it
out, so we should either move such fixtures out of code or make an exeption
for such type of code.

On Mon, Dec 7, 2015 at 1:03 PM, Igor Kalnitsky 
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. 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.
>
> 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.
>
>
> > 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? :)
>
> 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.
>
> - I
>
> On Mon, Dec 7, 2015 at 3:28 AM, Andrey Tykhonov 
> 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 

Re: [openstack-dev] [Fuel] Patch size limit

[Roman Prykhodchenko]

Maciej, thanks for bringing this topic up for the discussion!

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.

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.

Folks proposed two different solutions here:
  1. Set a CI job that puts a -1 to every patch that’s bigger than a certain 
limit.
  2. Make reviewers responsible for -1ing a patch.

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.

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:
  1. It can be split
  2. The design of the feature is completely wrong.

We need to set a treshold for a huge patch and block all patches that exceed it 
by putting -2 scores.

Summary:

- Let’s set two thresholds — one for to big and one for huge patches.
- Let’s create a job that will warn an appropriate core team if a too big patch 
is submitted and block huge patches.


- romcheg



> 7 груд. 2015 р. о 11:23 Stanislaw Bogatkin  
> написав(ла):
> 
> > 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? :)
> 
> I believe you should ask other people if you are right, as set '-1' to code 
> that you
> cannot improve is not the best option, so
> 
> 
> > If you are not sure how the improved code would look like, just let it go!
> is right
> 
> 
> Also I think that set some strict boundaries, like 400 LOC per patch is 
> wrong. For example, if you
> introduce new test fixture for fuel-library, it usually about 800+ LOC and 
> you can't split it
> out, so we should either move such fixtures out of code or make an exeption 
> for such type of code.
> 
> On Mon, Dec 7, 2015 at 1:03 PM, Igor Kalnitsky  > 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. 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.
> 
> 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.
> 
> 
> > 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? :)
> 
> 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.
> 
> - I
> 
> On Mon, Dec 7, 2015 at 3:28 AM, Andrey Tykhonov  > 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 

Re: [openstack-dev] [Fuel] Patch size limit

[Sergii Golovatiuk]

Hi,

On Mon, Dec 7, 2015 at 2:28 AM, Andrey Tykhonov 
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-requ...@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-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Fuel] Patch size limit

[Roman Prykhodchenko]

Following up on my previous email:

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.



> 7 груд. 2015 р. о 12:23 Roman Prykhodchenko  написав(ла):
> 
> Maciej, thanks for bringing this topic up for the discussion!
> 
> 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.
> 
> 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.
> 
> Folks proposed two different solutions here:
>   1. Set a CI job that puts a -1 to every patch that’s bigger than a certain 
> limit.
>   2. Make reviewers responsible for -1ing a patch.
> 
> 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.
> 
> 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:
>   1. It can be split
>   2. The design of the feature is completely wrong.
> 
> We need to set a treshold for a huge patch and block all patches that exceed 
> it by putting -2 scores.
> 
> Summary:
> 
> - Let’s set two thresholds — one for to big and one for huge patches.
> - Let’s create a job that will warn an appropriate core team if a too big 
> patch is submitted and block huge patches.
> 
> 
> - romcheg
> 
> 
> 
>> 7 груд. 2015 р. о 11:23 Stanislaw Bogatkin > > написав(ла):
>> 
>> > 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? :)
>> 
>> I believe you should ask other people if you are right, as set '-1' to code 
>> that you
>> cannot improve is not the best option, so
>> 
>> 
>> > If you are not sure how the improved code would look like, just let it go!
>> is right
>> 
>> 
>> Also I think that set some strict boundaries, like 400 LOC per patch is 
>> wrong. For example, if you
>> introduce new test fixture for fuel-library, it usually about 800+ LOC and 
>> you can't split it
>> out, so we should either move such fixtures out of code or make an exeption 
>> for such type of code.
>> 
>> On Mon, Dec 7, 2015 at 1:03 PM, Igor Kalnitsky > > 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. 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.
>> 
>> 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.
>> 
>> 
>> > 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? :)
>> 
>> 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.
>> 
>> - I
>> 
>> On Mon, Dec 7, 2015 at 3:28 AM, Andrey Tykhonov > > 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 

Re: [openstack-dev] [Fuel] Patch size limit

[Andrew Woodward]

On Mon, Dec 7, 2015 at 3:32 AM Roman Prykhodchenko  wrote:

> Maciej, thanks for bringing this topic up for the discussion!
>
> 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.
>
> 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.
>
> Folks proposed two different solutions here:
>   1. Set a CI job that puts a -1 to every patch that’s bigger than a
> certain limit.
>   2. Make reviewers responsible for -1ing a patch.
>
> 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.
>
> 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:
>   1. It can be split
>   2. The design of the feature is completely wrong.
>

> We need to set a treshold for a huge patch and block all patches that
> exceed it by putting -2 scores.
>
> Summary:
>
> - Let’s set two thresholds — one for to big and one for huge patches.
> - Let’s create a job that will warn an appropriate core team if a too big
> patch is submitted and block huge patches.
>

On it's own, LOC is invalid as the metric for determining if a patch it too
large (its a good indicator that it might be too large but it does require
more datapoints). It can be easily obscured just with moving code around
and tests. While lare reviews can often be split This can, and should be
directly up to the reviewers to determine if it's necessary to split a
patch. At the same time it's not a unilateral decision on the reviewer's
part if it needs to be split. Like the code review in general, it needs to
be worked out between the quorum of the submitter and the reviewers. This
is the entire point of peer review and creating an auto-mated response to
something that is the responsibility of the review process seem silly. At
the same time, I'm not opposed to a code quality guideline that points out
that if your review is over X large its probably too big, and should be
split. Pointing out that this is an area of contention will help the
reviewer better prepare

For example our NOOP test yamls are huge [1] is 1291 lines. Simply adding
one set of data could easily become 4000+ lines (for example neutron vlan
l3 ha with ceph and ceilometer are 4604 lines.

[1]
https://github.com/openstack/fuel-library/blob/master/tests/noop/astute.yaml/neut_vlan.ceph.controller-ephemeral-ceph.yaml


> - romcheg
>
>
>
> 7 груд. 2015 р. о 11:23 Stanislaw Bogatkin 
> написав(ла):
>
>
> > 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? :)
>
> I believe you should ask other people if you are right, as set '-1' to
> code that you
> cannot improve is not the best option, so
>
>
> > If you are not sure how the improved code would look like, just let it
> go!
> is right
>
>
> Also I think that set some strict boundaries, like 400 LOC per patch is
> wrong. For example, if you
> introduce new test fixture for fuel-library, it usually about 800+ LOC and
> you can't split it
> out, so we should either move such fixtures out of code or make an
> exeption for such type of code.
>
> On Mon, Dec 7, 2015 at 1:03 PM, Igor Kalnitsky 
> 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. 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.
>>
>> 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.
>>
>>
>> > If you 

Re: [openstack-dev] [Fuel] Patch size limit

[Andrey Tykhonov]

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 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-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Fuel] Patch size limit

[Igor Kalnitsky]

Hey folks,

I agree that patches must be as small as possible. I believe it will
significantly increase our review experience - more fast review, and,
therefore, landing to master.

However, I don't agree that we should introduce criteria based on LOC,
because of mentioned reasons above. I believe that patches must be
atomic, no matter how much LOC it has. In the same time, we must not
have the whole feature as atomic unit here.

So basically my points are:

* Let's do not go with strict LOC. Decision it's ok to go with one
patch or not, should be up to code reviewers.
* If reviewer thinks that patch could and should be splitted into few,
then he/she set -1 and ask contributor to split it.
* Reviewers shouldn't hesitate to set -1 and ask to split patch.

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Fuel] Patch size limit

[Michael Krotscheck]

TL/DR: I think you're trying to solve the problem the wrong way. If you're
trying to reduce the burden of large patches, I feel the project in
question should distribute the burden of reviewing the big ones so one
person's not stuck doing them all. That also means you can keep that patch
in mental context.

Also, random concerns I have with using "Line of code" as a metric:

* What counts as a Line of Code? Does whitespace count? Comments? Docs?
Tests? Should comments/docs/tests be included in this heuristic?
* Taking a big patch, and splitting it into several, can easily lead to
dead code as the first patch may remain completely inactive on master until
the entire patch chain merges.
* git annotate becomes far less useful, as you cannot simply look at all
the applicable changes for a given patch - you have to dig for all related
patches.
* Reverting things becomes more difficult for the same reason. Would you
revert all-in-one? One revert per patch?

I've seen, and written, 1000+ line patches. Some of them were 200 lines of
logic, 1000+ lines of tests. Others included extensive documentation,
comments, etc, or perhaps-too-verbose parameter and method names that
clearly explain what they do. Others use method-parameter-per-line style
formatting in their code to assist in legibility.

While I totally understand how frustrating it is to have to review a large
patch, I'm not in favor of a hard limit for something which is governed
mostly by whitespace and formatting preferences.

Michael

On Tue, Dec 1, 2015 at 6:36 AM Sylwester Brzeczkowski <
sbrzeczkow...@mirantis.com> wrote:

> Neil, just to clarify: moved/renamed files are marked as "R" so I think
> there may be some way to ignore such files when counting LOC.
>
> Maciej, I completely agree with you. It's pretty hard to review such big
> change,and takes a lot of time which could be saved by submitting smaller
> patches.
>
> +1
>
> On Tue, Dec 1, 2015 at 1:50 PM, Neil Jerram 
> wrote:
>
>> On 01/12/15 12:45, Maciej Kwiek wrote:
>> > Hi,
>> >
>> > I recently noticed the influx of big patches hitting Gerrit
>> > (especially in fuel-web, but I also heard that there was a couple of
>> > big ones in library). I think that patches that have 1000 LOC are
>> > simply too big to review thoroughly and reliably.
>> >
>> > I would argue that there should be a limit to patch size, either a
>> > soft one (i.e. written down in contributor guidelines) or a hard one
>> > (e.g. enforced by gate job).
>> >
>> > I think that we need a discussion whether we really need this limit,
>> > what should it be, and how to enforce it.
>> >
>> > I personally think that most patches that are over 400 LOC could be
>> > easily split into at least two smaller changes.
>> >
>> > Regarding the limit enforcement - I think we should go with the soft
>> > limit, with X LOC written as a guideline and giving the reviewers a
>> > possibility to give -1s to patches that are too big, but also giving
>> > the possibility to merge bigger changes if it's absolutely necessary
>> > (in really rare cases the changes simply cannot be split). We may mix
>> > in the hard limit for ridiculously large patches (twice the "soft
>> > limit" would be good in my opinion), so the gate would automatically
>> > reject such patches, forcing contributor to split his patch.
>> >
>> > Please share your thoughts on this.
>>
>> I think most of your principle is correct.  However I can imagine a file
>> renaming / moving patch that would appear in Gerrit to be >=1000 LOC,
>> but would actually just be file moves, with perhaps some trivial changes
>> to Python module paths; and I don't think it would be helpful to force a
>> patch like that to be split up.  So it may not be correct to enforce a
>> hard limit all the time.
>>
>> Neil
>>
>>
>> __
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe:
>> openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>
>
>
> --
> *Sylwester Brzeczkowski*
> Junior Python Software Engineer
> Product Development-Core : Product Engineering
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@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-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Fuel] Patch size limit

[Sergii Golovatiuk]

Hi,


On Tue, Dec 1, 2015 at 5:41 PM, Michael Krotscheck 
wrote:

> TL/DR: I think you're trying to solve the problem the wrong way. If you're
> trying to reduce the burden of large patches, I feel the project in
> question should distribute the burden of reviewing the big ones so one
> person's not stuck doing them all. That also means you can keep that patch
> in mental context.
>
> Also, random concerns I have with using "Line of code" as a metric:
>
> * What counts as a Line of Code? Does whitespace count? Comments? Docs?
> Tests? Should comments/docs/tests be included in this heuristic?
>

Comments/tests can be easily excluded from scope of LOC calculation. The
number of lines may be negotiated with community. Rally has 500 lines limit
in CI. Fuel community may vote for 600 or for 400 and change it later. It's
not static.

* Taking a big patch, and splitting it into several, can easily lead to
> dead code as the first patch may remain completely inactive on master until
> the entire patch chain merges.
>

Dependent patches on gerrit is a good sample how big patch should be split.
Some of them can break CI though the chain gives better view than one
massive patch.


> * git annotate becomes far less useful, as you cannot simply look at all
> the applicable changes for a given patch - you have to dig for all related
> patches.
> * Reverting things becomes more difficult for the same reason. Would you
> revert all-in-one? One revert per patch?
>

It's just a strategy. Usually revert all-in-one works. However, I saw when
patch in the middle was reverted.


> I've seen, and written, 1000+ line patches. Some of them were 200 lines of
> logic, 1000+ lines of tests. Others included extensive documentation,
> comments, etc, or perhaps-too-verbose parameter and method names that
> clearly explain what they do. Others use method-parameter-per-line style
> formatting in their code to assist in legibility.
>

>
> While I totally understand how frustrating it is to have to review a large
> patch, I'm not in favor of a hard limit for something which is governed
> mostly by whitespace and formatting preferences.
>

There are some cases when patch cannot be split. However, Core reviewer may
merge with -1 from CI LOC job in that case. Also the author may specify in
commit message why the patch cannot be split. Though in that case it will
be author's responsibility to prove the necessity.


> Michael
>
> On Tue, Dec 1, 2015 at 6:36 AM Sylwester Brzeczkowski <
> sbrzeczkow...@mirantis.com> wrote:
>
>> Neil, just to clarify: moved/renamed files are marked as "R" so I think
>> there may be some way to ignore such files when counting LOC.
>>
>> Maciej, I completely agree with you. It's pretty hard to review such big
>> change,and takes a lot of time which could be saved by submitting smaller
>> patches.
>>
>> +1
>>
>> On Tue, Dec 1, 2015 at 1:50 PM, Neil Jerram 
>> wrote:
>>
>>> On 01/12/15 12:45, Maciej Kwiek wrote:
>>> > Hi,
>>> >
>>> > I recently noticed the influx of big patches hitting Gerrit
>>> > (especially in fuel-web, but I also heard that there was a couple of
>>> > big ones in library). I think that patches that have 1000 LOC are
>>> > simply too big to review thoroughly and reliably.
>>> >
>>> > I would argue that there should be a limit to patch size, either a
>>> > soft one (i.e. written down in contributor guidelines) or a hard one
>>> > (e.g. enforced by gate job).
>>> >
>>> > I think that we need a discussion whether we really need this limit,
>>> > what should it be, and how to enforce it.
>>> >
>>> > I personally think that most patches that are over 400 LOC could be
>>> > easily split into at least two smaller changes.
>>> >
>>> > Regarding the limit enforcement - I think we should go with the soft
>>> > limit, with X LOC written as a guideline and giving the reviewers a
>>> > possibility to give -1s to patches that are too big, but also giving
>>> > the possibility to merge bigger changes if it's absolutely necessary
>>> > (in really rare cases the changes simply cannot be split). We may mix
>>> > in the hard limit for ridiculously large patches (twice the "soft
>>> > limit" would be good in my opinion), so the gate would automatically
>>> > reject such patches, forcing contributor to split his patch.
>>> >
>>> > Please share your thoughts on this.
>>>
>>> I think most of your principle is correct.  However I can imagine a file
>>> renaming / moving patch that would appear in Gerrit to be >=1000 LOC,
>>> but would actually just be file moves, with perhaps some trivial changes
>>> to Python module paths; and I don't think it would be helpful to force a
>>> patch like that to be split up.  So it may not be correct to enforce a
>>> hard limit all the time.
>>>
>>> Neil
>>>
>>>
>>>
>>> __
>>> OpenStack Development Mailing List (not for usage questions)
>>> Unsubscribe:
>>> 

Re: [openstack-dev] [Fuel] Patch size limit

[Neil Jerram]

On 01/12/15 12:45, Maciej Kwiek wrote:
> Hi,
>
> I recently noticed the influx of big patches hitting Gerrit
> (especially in fuel-web, but I also heard that there was a couple of
> big ones in library). I think that patches that have 1000 LOC are
> simply too big to review thoroughly and reliably.
>
> I would argue that there should be a limit to patch size, either a
> soft one (i.e. written down in contributor guidelines) or a hard one
> (e.g. enforced by gate job).
>
> I think that we need a discussion whether we really need this limit,
> what should it be, and how to enforce it.
>
> I personally think that most patches that are over 400 LOC could be
> easily split into at least two smaller changes.
>
> Regarding the limit enforcement - I think we should go with the soft
> limit, with X LOC written as a guideline and giving the reviewers a
> possibility to give -1s to patches that are too big, but also giving
> the possibility to merge bigger changes if it's absolutely necessary
> (in really rare cases the changes simply cannot be split). We may mix
> in the hard limit for ridiculously large patches (twice the "soft
> limit" would be good in my opinion), so the gate would automatically
> reject such patches, forcing contributor to split his patch.
>
> Please share your thoughts on this.

I think most of your principle is correct.  However I can imagine a file
renaming / moving patch that would appear in Gerrit to be >=1000 LOC,
but would actually just be file moves, with perhaps some trivial changes
to Python module paths; and I don't think it would be helpful to force a
patch like that to be split up.  So it may not be correct to enforce a
hard limit all the time.

Neil


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Fuel] Patch size limit

[Sylwester Brzeczkowski]

Neil, just to clarify: moved/renamed files are marked as "R" so I think
there may be some way to ignore such files when counting LOC.

Maciej, I completely agree with you. It's pretty hard to review such big
change,and takes a lot of time which could be saved by submitting smaller
patches.

+1

On Tue, Dec 1, 2015 at 1:50 PM, Neil Jerram 
wrote:

> On 01/12/15 12:45, Maciej Kwiek wrote:
> > Hi,
> >
> > I recently noticed the influx of big patches hitting Gerrit
> > (especially in fuel-web, but I also heard that there was a couple of
> > big ones in library). I think that patches that have 1000 LOC are
> > simply too big to review thoroughly and reliably.
> >
> > I would argue that there should be a limit to patch size, either a
> > soft one (i.e. written down in contributor guidelines) or a hard one
> > (e.g. enforced by gate job).
> >
> > I think that we need a discussion whether we really need this limit,
> > what should it be, and how to enforce it.
> >
> > I personally think that most patches that are over 400 LOC could be
> > easily split into at least two smaller changes.
> >
> > Regarding the limit enforcement - I think we should go with the soft
> > limit, with X LOC written as a guideline and giving the reviewers a
> > possibility to give -1s to patches that are too big, but also giving
> > the possibility to merge bigger changes if it's absolutely necessary
> > (in really rare cases the changes simply cannot be split). We may mix
> > in the hard limit for ridiculously large patches (twice the "soft
> > limit" would be good in my opinion), so the gate would automatically
> > reject such patches, forcing contributor to split his patch.
> >
> > Please share your thoughts on this.
>
> I think most of your principle is correct.  However I can imagine a file
> renaming / moving patch that would appear in Gerrit to be >=1000 LOC,
> but would actually just be file moves, with perhaps some trivial changes
> to Python module paths; and I don't think it would be helpful to force a
> patch like that to be split up.  So it may not be correct to enforce a
> hard limit all the time.
>
> Neil
>
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>



-- 
*Sylwester Brzeczkowski*
Junior Python Software Engineer
Product Development-Core : Product Engineering
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Fuel] Patch size limit

[Evgeniy L]

Hi Maciej, thank you for bringing this up,

+1, but we should discuss the limit, personally for me it's ok to review
400loc patches,
if the patch covers only one bug-fix/feature implementation.

So if everybody is agree, we should:
1. update contribution guide
2. create a task for *non-voting* gate, which will -1 patch if it has loc
>= x (400?),
also as Neil mentioned, it shouldn't take into account files renaming.

Thanks,

On Tue, Dec 1, 2015 at 3:40 PM, Maciej Kwiek  wrote:

> Hi,
>
> I recently noticed the influx of big patches hitting Gerrit (especially in
> fuel-web, but I also heard that there was a couple of big ones in library).
> I think that patches that have 1000 LOC are simply too big to review
> thoroughly and reliably.
>
> I would argue that there should be a limit to patch size, either a soft
> one (i.e. written down in contributor guidelines) or a hard one (e.g.
> enforced by gate job).
>
> I think that we need a discussion whether we really need this limit, what
> should it be, and how to enforce it.
>
> I personally think that most patches that are over 400 LOC could be easily
> split into at least two smaller changes.
>
> Regarding the limit enforcement - I think we should go with the soft
> limit, with X LOC written as a guideline and giving the reviewers a
> possibility to give -1s to patches that are too big, but also giving the
> possibility to merge bigger changes if it's absolutely necessary (in really
> rare cases the changes simply cannot be split). We may mix in the hard
> limit for ridiculously large patches (twice the "soft limit" would be good
> in my opinion), so the gate would automatically reject such patches,
> forcing contributor to split his patch.
>
> Please share your thoughts on this.
>
> Regards,
> Maciej Kwiek
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@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-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev