I don't think reviews should ever be split up so that one review contributes functionality and another contributes tests. If Gerrit is going to run a build on a review, the build should test the new functionality added by the review. In my experience, the IP review process is usually very fast so the benefit of having both the functionality and the tests in a single change, which can be cherry-picked to other branches, reverted, etc. far outweighs the slight delay imposed by the IP review. I do encourage contributors to split large reviews into smaller chunks just to make them easier to review, but each chunk should be a cohesive piece of functionality including tests.
Sam -- Sam Davis Software Engineer, Tasktop Dev Committer, Eclipse Mylyn http://tasktop.com On Thu, Nov 19, 2015 at 7:17 AM, Doug Schaefer <[email protected]> wrote: > We need to keep in mind the spirit of the rule which is as Pascal > mentioned. I always thought the lines of code was a guideline and certainly > doesn’t excuse the committer from taking a good look at the code and even > if it’s less than 1000 lines, if it looks like a algorithm that could be > taken from somewhere else that it gets reviewed. > > From: <[email protected]> on behalf of Fred > Bricon <[email protected]> > Reply-To: Cross project issues <[email protected]> > Date: Thursday, November 19, 2015 at 10:12 AM > To: Cross project issues <[email protected]> > Subject: Re: [cross-project-issues-dev] 1000 line limit for contributions > > I'm not sure how the workload from the IP team is split, between running > automated IP checks (blackduck or similar) and manual work. But I think > it'd be interesting if at least the automated part could be included as > part of a CI build checking gerrit contributions. That'd would surely > offload the IP team work significantly. > > On Thu, Nov 19, 2015 at 10:01 AM, Pascal Rapicault <[email protected]> > wrote: > >> It is my understanding that the 1000 lines limit is here to prevent code >> to be copied from an external place and be added to Eclipse. This is a >> good measure. >> However, I think there is a number of contributions where the code has >> obviously not been copied. For example, this is the case with the >> contribution Jan mentioned. There is no way the code could be coming from >> somewhere else. It is way too specific to Tycho and the feature being added. >> >> IMO, the IP process could be relaxed and thus the IP teamwork load >> reduced, if committers were trusted on what to put through the IP process >> and what not. >> >> With such a rule, a committer who has doubts about 200 lines of code >> could take it through the IP process, and yet for things that are obviously >> "new" code, would not. IMO, such a process based on committer trust could >> help focus the efforts of the IP team on things that are potentially >> problematic. >> >> >> >> >> On 15-11-19 05:47 AM, Ed Merks wrote: >> >>> Yes, I believe it's an important aspect of Eclipse that makes it stand >>> out as the best place to be if you want the broadest possible community of >>> adopters. Of course this benefit doesn't come without a cost and of >>> course that can be frustrating. In a specific case of a contribution that >>> consists of a relatively smaller changes to the framework/tool with a >>> relatively larger addition of test case(s), it would seem reasonable to >>> split the two, if it's important that the change to the tools/framework >>> show up as quickly as possible. >>> >>> I certainly don't suggest gaming the system, though I do tend to point >>> out to the IP committee all the ways it can be gamed, and will be gamed by >>> developers who are frustrated and don't take the issue seriously. I ask >>> questions such as how long can a line be? One can fit quite a lot on a line >>> line and reformat it later. Also, why should a blank line count for >>> anything? Is a line with just a curly brace on it really IP? And yes, of >>> course I make them aware that contributions can be split into smaller >>> chunks... >>> >>> Perhaps this specific review period overlapped with EclispeCon Europe >>> where we had the pleasure of spending personal time with the with the IP >>> staff... >>> >>> >>> On 19/11/2015 11:31 AM, Christian Campo wrote: >>> >>>> Wouldnt it be worth to hear what the IP Team has to say why this took so >>>> long ? I see that Sharon appologized on the CQ that it took so long. >>>> That >>>> made me believe that this was an exception. >>>> >>>> Does every CQ with 1000 lines take so long ? What is the experience of >>>> others about reviews with code contributions. >>>> As I remember vaguely (and that might be incorrect) the IP team runs >>>> automatic scans over the code, but I am not sure what else they do. >>>> >>>> I for once believe the work of the IP Team is important and one of the >>>> core values of the EF vs say Github and I take it serious. >>>> >>>> Just my 2 cents >>>> >>>> christian >>>> >>>> Am 19.11.15, 11:22 schrieb " >>>> [email protected] >>>> on behalf of Sievers, Jan" unter >>>> <[email protected] on behalf of >>>> [email protected]>: >>>> >>>> If everybody tells me there are ways to dodge around that rule (and of >>>>> course I know there are), the question arises why do we have the rule >>>>> in >>>>> the first place. Seems a little absurd to me. >>>>> >>>>> the effort is not minimal if I have to artificially split up commits. >>>>> Or maybe you expect me to explain to contributors: >>>>> >>>>> "look, we have this process but nobody takes it serious anyway. so >>>>> please >>>>> split up your commit into several < 1000 LOC chunks" ? >>>>> >>>>> Best Regards, >>>>> Jan >>>>> >>>>> >>>>> >>>>> On 19/11/15 11:00, "[email protected] on >>>>> behalf of Ed Willink" <[email protected] on >>>>> behalf of [email protected]> wrote: >>>>> >>>>> Hi >>>>>> >>>>>> Presumably you put tests in a separate plugin, so splitting off the >>>>>> tests as a separate contribution gets you twice the limit with minimal >>>>>> effort. >>>>>> >>>>>> Perhaps a 10000 line limit might be appropriate for non-deliverable >>>>>> code >>>>>> such as tests and build tools. >>>>>> >>>>>> Regards >>>>>> >>>>>> Ed Willink >>>>>> >>>>>> >>>>>> >>>>>> On 19/11/2015 09:49, Sievers, Jan wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> in the course of >>>>>>> >>>>>>> https://bugs.eclipse.org/bugs/show_bug.cgi?id=477328 >>>>>>> >>>>>>> >>>>>>> we had a contribution that slightly exceeded 1000 lines and thus >>>>>>> needed a CQ. >>>>>>> It took about one month to review it. >>>>>>> >>>>>>> I am sure the legal team does its very best to keep up with the load, >>>>>>> so the following is in no way a criticism of the >>>>>>> people who actually do the legal review. >>>>>>> >>>>>>> Rather take it as food for thought to whoever set up this rule. >>>>>>> >>>>>>> IMHO the 1000 line rule is effectively setting the wrong incentives >>>>>>> for a thriving opensource project. >>>>>>> >>>>>>> Here is why I think so: >>>>>>> >>>>>>> >>>>>>> The most diligent contributors add a lot of tests to their patch to >>>>>>> prove it works. >>>>>>> This is a good thing and we actively encourage contributors to >>>>>>> thoroughly test. >>>>>>> Test code can easily outweigh productive code being tested in terms >>>>>>> of >>>>>>> LOC. >>>>>>> However this means the most diligent contributors, i.e. the ones you >>>>>>> want to attract, are more likely to hit the 1000 line limit. >>>>>>> Instead of thanking them for their hard work, we effectively punish >>>>>>> them with an extra month or more wait time before their patch can be >>>>>>> merged. >>>>>>> Apart from that, the 1000 line limit seems arbitrary to me because >>>>>>> technically you can split up any commit into any number >>>>>>> of smaller commits below the 1000 line limit. >>>>>>> >>>>>>> Best Regards, >>>>>>> Jan >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> cross-project-issues-dev mailing list >>>>>>> [email protected] >>>>>>> To change your delivery options, retrieve your password, or >>>>>>> unsubscribe from this list, visit >>>>>>> https://dev.eclipse.org/mailman/listinfo/cross-project-issues-dev >>>>>>> >>>>>>> _______________________________________________ >>>>>> cross-project-issues-dev mailing list >>>>>> [email protected] >>>>>> To change your delivery options, retrieve your password, or >>>>>> unsubscribe >>>>>> >>>>> >from this list, visit >>>>> >>>>>> https://dev.eclipse.org/mailman/listinfo/cross-project-issues-dev >>>>>> >>>>> _______________________________________________ >>>>> cross-project-issues-dev mailing list >>>>> [email protected] >>>>> To change your delivery options, retrieve your password, or unsubscribe >>>>> >>>> >from this list, visit >>>> >>>>> https://dev.eclipse.org/mailman/listinfo/cross-project-issues-dev >>>>> >>>> ------------------------------------------------------------- >>>> compeople AG >>>> Untermainanlage 8 >>>> 60329 Frankfurt/Main >>>> fon: +49 (0) 69 / 27 22 18 0 >>>> fax: +49 (0) 69 / 27 22 18 22 >>>> web: www.compeople.de >>>> >>>> Vorstand: Jürgen Wiesmaier >>>> Aufsichtsratsvorsitzender: Christian Glanz >>>> >>>> Sitz der Gesellschaft: Frankfurt/Main >>>> Handelsregister Frankfurt HRB 56759 >>>> USt-IdNr. DE207665352 >>>> ------------------------------------------------------------- >>>> _______________________________________________ >>>> cross-project-issues-dev mailing list >>>> [email protected] >>>> To change your delivery options, retrieve your password, or unsubscribe >>>> from this list, visit >>>> https://dev.eclipse.org/mailman/listinfo/cross-project-issues-dev >>>> >>> >>> _______________________________________________ >>> cross-project-issues-dev mailing list >>> [email protected] >>> To change your delivery options, retrieve your password, or unsubscribe >>> from this list, visit >>> https://dev.eclipse.org/mailman/listinfo/cross-project-issues-dev >>> >> >> _______________________________________________ >> cross-project-issues-dev mailing list >> [email protected] >> To change your delivery options, retrieve your password, or unsubscribe >> from this list, visit >> https://dev.eclipse.org/mailman/listinfo/cross-project-issues-dev >> > > > > -- > "Have you tried turning it off and on again" - The IT Crowd > And if that fails, then http://goo.gl/tnBgH5 > > > _______________________________________________ > cross-project-issues-dev mailing list > [email protected] > To change your delivery options, retrieve your password, or unsubscribe > from this list, visit > https://dev.eclipse.org/mailman/listinfo/cross-project-issues-dev >
_______________________________________________ cross-project-issues-dev mailing list [email protected] To change your delivery options, retrieve your password, or unsubscribe from this list, visit https://dev.eclipse.org/mailman/listinfo/cross-project-issues-dev
