Open Stack

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 <mkwiek at mirantis.com> 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-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/20151201/2480e970/attachment.html>