Open Stack

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 <Neil.Jerram at metaswitch.com>
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-request at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20151201/94220ed2/attachment.html>