<div dir="ltr"><div><div>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.<br><br></div>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.<br><br></div>+1<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Dec 1, 2015 at 1:50 PM, Neil Jerram <span dir="ltr"><<a href="mailto:Neil.Jerram@metaswitch.com" target="_blank">Neil.Jerram@metaswitch.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 01/12/15 12:45, Maciej Kwiek wrote:<br>
> Hi,<br>
><br>
> I recently noticed the influx of big patches hitting Gerrit<br>
> (especially in fuel-web, but I also heard that there was a couple of<br>
> big ones in library). I think that patches that have 1000 LOC are<br>
> simply too big to review thoroughly and reliably.<br>
><br>
> I would argue that there should be a limit to patch size, either a<br>
> soft one (i.e. written down in contributor guidelines) or a hard one<br>
> (e.g. enforced by gate job).<br>
><br>
> I think that we need a discussion whether we really need this limit,<br>
> what should it be, and how to enforce it.<br>
><br>
> I personally think that most patches that are over 400 LOC could be<br>
> easily split into at least two smaller changes.<br>
><br>
> Regarding the limit enforcement - I think we should go with the soft<br>
> limit, with X LOC written as a guideline and giving the reviewers a<br>
> possibility to give -1s to patches that are too big, but also giving<br>
> the possibility to merge bigger changes if it's absolutely necessary<br>
> (in really rare cases the changes simply cannot be split). We may mix<br>
> in the hard limit for ridiculously large patches (twice the "soft<br>
> limit" would be good in my opinion), so the gate would automatically<br>
> reject such patches, forcing contributor to split his patch.<br>
><br>
> Please share your thoughts on this.<br>
<br>
</span>I think most of your principle is correct.  However I can imagine a file<br>
renaming / moving patch that would appear in Gerrit to be >=1000 LOC,<br>
but would actually just be file moves, with perhaps some trivial changes<br>
to Python module paths; and I don't think it would be helpful to force a<br>
patch like that to be split up.  So it may not be correct to enforce a<br>
hard limit all the time.<br>
<br>
    Neil<br>
<br>
<br>
__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature"><div dir="ltr"><div><b>Sylwester Brzeczkowski</b><br></div>Junior Python Software <span>Engineer</span><br>Product Development-Core : Product Engineering<br></div></div>
</div>