----- Original Message ----- > From: "Doron Fediuck" <[email protected]> > To: "Eyal Edri" <[email protected]> > Cc: "engine-devel" <[email protected]> > Sent: Sunday, January 6, 2013 4:30:23 PM > Subject: Re: [Engine-devel] Java code formatting > > > ----- Original Message ----- > > From: "Eyal Edri" <[email protected]> > > To: "engine-devel" <[email protected]> > > Sent: Friday, January 4, 2013 5:39:52 PM > > Subject: Re: [Engine-devel] Java code formatting > > > > > > > > ----- Original Message ----- > > > From: "David Jaša" <[email protected]> > > > To: "engine-devel" <[email protected]> > > > Sent: Friday, January 4, 2013 4:37:13 PM > > > Subject: Re: [Engine-devel] Java code formatting > > > > > > Doron Fediuck píše v St 02. 01. 2013 v 03:24 -0500: > > > > > > > > ----- Original Message ----- > > > > > From: "Livnat Peer" <[email protected]> > > > > > To: "Doron Fediuck" <[email protected]> > > > > > Cc: "engine-devel" <[email protected]> > > > > > Sent: Tuesday, January 1, 2013 5:58:15 PM > > > > > Subject: Re: [Engine-devel] Java code formatting > > > > > > > > > > On 01/01/2013 05:39 PM, Doron Fediuck wrote: > > > > > > > > > > > > > > > > > > ----- Original Message ----- > > > > > >> From: "Alon Bar-Lev" <[email protected]> > > > > > >> To: "Doron Fediuck" <[email protected]> > > > > > >> Cc: "engine-devel" <[email protected]> > > > > > >> Sent: Tuesday, January 1, 2013 5:05:09 PM > > > > > >> Subject: Re: [Engine-devel] Java code formatting > > > > > >> > > > > > >> > > > > > >> > > > > > >> ----- Original Message ----- > > > > > >>> From: "Doron Fediuck" <[email protected]> > > > > > >>> To: "Alon Bar-Lev" <[email protected]> > > > > > >>> Cc: "engine-devel" <[email protected]> > > > > > >>> Sent: Tuesday, January 1, 2013 5:00:20 PM > > > > > >>> Subject: Re: [Engine-devel] Java code formatting > > > > > >>> > > > > > >>> > > > > > >>> > > > > > >>> ----- Original Message ----- > > > > > >>>> From: "Alon Bar-Lev" <[email protected]> > > > > > >>>> To: "Doron Fediuck" <[email protected]> > > > > > >>>> Cc: "engine-devel" <[email protected]> > > > > > >>>> Sent: Tuesday, January 1, 2013 4:53:49 PM > > > > > >>>> Subject: Re: [Engine-devel] Java code formatting > > > > > >>>> > > > > > >>>> > > > > > >>>> > > > > > >>>> ----- Original Message ----- > > > > > >>>>> From: "Doron Fediuck" <[email protected]> > > > > > >>>>> To: "Alon Bar-Lev" <[email protected]> > > > > > >>>>> Cc: "engine-devel" <[email protected]> > > > > > >>>>> Sent: Tuesday, January 1, 2013 4:48:22 PM > > > > > >>>>> Subject: Re: [Engine-devel] Java code formatting > > > > > >>>>> > > > > > >>>>> > > > > > >>>>> > > > > > >>>>> ----- Original Message ----- > > > > > >>>>>> From: "Alon Bar-Lev" <[email protected]> > > > > > >>>>>> To: "Doron Fediuck" <[email protected]> > > > > > >>>>>> Cc: "engine-devel" <[email protected]> > > > > > >>>>>> Sent: Tuesday, January 1, 2013 4:33:20 PM > > > > > >>>>>> Subject: Re: [Engine-devel] Java code formatting > > > > > >>>>>> > > > > > >>>>>> > > > > > >>>>>> > > > > > >>>>>> ----- Original Message ----- > > > > > >>>>>>> From: "Doron Fediuck" <[email protected]> > > > > > >>>>>>> To: "Alon Bar-Lev" <[email protected]> > > > > > >>>>>>> Cc: "engine-devel" <[email protected]> > > > > > >>>>>>> Sent: Tuesday, January 1, 2013 4:28:15 PM > > > > > >>>>>>> Subject: Re: [Engine-devel] Java code formatting > > > > > >>>>>>> > > > > > >>>>>>> > > > > > >>>>>>> > > > > > >>>>>>> ----- Original Message ----- > > > > > >>>>>>>> From: "Alon Bar-Lev" <[email protected]> > > > > > >>>>>>>> To: "Doron Fediuck" <[email protected]> > > > > > >>>>>>>> Cc: "engine-devel" <[email protected]> > > > > > >>>>>>>> Sent: Tuesday, January 1, 2013 4:17:18 PM > > > > > >>>>>>>> Subject: Re: [Engine-devel] Java code formatting > > > > > >>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>>> ----- Original Message ----- > > > > > >>>>>>>>> From: "Doron Fediuck" <[email protected]> > > > > > >>>>>>>>> To: "engine-devel" <[email protected]> > > > > > >>>>>>>>> Sent: Tuesday, January 1, 2013 4:07:53 PM > > > > > >>>>>>>>> Subject: [Engine-devel] Java code formatting > > > > > >>>>>>>>> > > > > > >>>>>>>>> Hi, > > > > > >>>>>>>>> Recently I saw many patches with multiple code > > > > > >>>>>>>>> re-formatting. > > > > > >>>>>>>>> When looking into it, we saw that many people > > > > > >>>>>>>>> didn't > > > > > >>>>>>>>> use > > > > > >>>>>>>>> the > > > > > >>>>>>>>> project > > > > > >>>>>>>>> policy, and now we have many files with bad > > > > > >>>>>>>>> formatting. > > > > > >>>>>>>>> > > > > > >>>>>>>>> So I just posted a big ugly fix for this[1], and > > > > > >>>>>>>>> hopefully > > > > > >>>>>>>>> if > > > > > >>>>>>>>> accepted > > > > > >>>>>>>>> people should start using the right conventions and > > > > > >>>>>>>>> reduce > > > > > >>>>>>>>> the > > > > > >>>>>>>>> amount > > > > > >>>>>>>>> of non-relevant changes we see in the patches. > > > > > >>>>>>>>> > > > > > >>>>>>>>> I'm aware of the fact that this may create some > > > > > >>>>>>>>> issues > > > > > >>>>>>>>> when > > > > > >>>>>>>>> porting > > > > > >>>>>>>>> patches, but better sooner than later. > > > > > >>>>>>>>> Doron. > > > > > >>>>>>>>> > > > > > >>>>>>>>> [1] http://gerrit.ovirt.org/#/c/10541/1 > > > > > >>>>>>>> > > > > > >>>>>>>> Hi, > > > > > >>>>>>>> > > > > > >>>>>>>> These automatic conversions are not better than > > > > > >>>>>>>> current > > > > > >>>>>>>> state, > > > > > >>>>>>>> also > > > > > >>>>>>>> I > > > > > >>>>>>>> don't think that this is that important. If you want > > > > > >>>>>>>> machine > > > > > >>>>>>>> written > > > > > >>>>>>>> code, then also provide commit hook to reformat > > > > > >>>>>>>> anything, > > > > > >>>>>>>> and > > > > > >>>>>>>> probably machines to read it. > > > > > >>>>>>>> > > > > > >>>>>>>> I, personally, think that this change over the > > > > > >>>>>>>> sources > > > > > >>>>>>>> I > > > > > >>>>>>>> manage > > > > > >>>>>>>> did > > > > > >>>>>>>> not do any good. > > > > > >>>>>>>> > > > > > >>>>>>>> Regards, > > > > > >>>>>>>> Alon > > > > > >>>>>>> > > > > > >>>>>>> Alon, > > > > > >>>>>>> there's a formatting convention for the project set > > > > > >>>>>>> long > > > > > >>>>>>> ago. > > > > > >>>>>>> If you feel it needs to be fixed, go ahead and > > > > > >>>>>>> suggest > > > > > >>>>>>> a > > > > > >>>>>>> fix > > > > > >>>>>>> for > > > > > >>>>>>> the xml. > > > > > >>>>>>> Otherwise we end up in the current chaos, where every > > > > > >>>>>>> 2nd > > > > > >>>>>>> or > > > > > >>>>>>> 3rd > > > > > >>>>>>> patch carries unneeded changes. > > > > > >>>>>>> > > > > > >>>>>> > > > > > >>>>>> What do you mean unneeded changes? how do you prevent > > > > > >>>>>> this > > > > > >>>>>> in > > > > > >>>>>> future? > > > > > >>>>>> > > > > > >>>>>> Alon > > > > > >>>>> > > > > > >>>>> Unneeded changes is when you get one line of code fixed > > > > > >>>>> due > > > > > >>>>> to > > > > > >>>>> a > > > > > >>>>> bug, > > > > > >>>>> and many others re-indented. > > > > > >>>>> Best prevention is if people would make sure to use the > > > > > >>>>> same > > > > > >>>>> conventions. > > > > > >>>>> We also have a checkstyle which monitors important > > > > > >>>>> issues > > > > > >>>>> such > > > > > >>>>> as > > > > > >>>>> trailing > > > > > >>>>> white spaces, localization, etc. > > > > > >>>>> > > > > > >>>> > > > > > >>>> But how do you prevent this in future, not all working > > > > > >>>> in > > > > > >>>> same > > > > > >>>> editor > > > > > >>>> nor same styles. > > > > > >>>> You can say that once in a while you perform cross over > > > > > >>>> auto > > > > > >>>> re-format... > > > > > >>>> I am not native Java programmer but this sounds very > > > > > >>>> strange > > > > > >>>> thing > > > > > >>>> to > > > > > >>>> do, I don't think I know of any project that does that. > > > > > >>>> > > > > > >>>> The main problem is that people commit stuff they don't > > > > > >>>> touch > > > > > >>>> due > > > > > >>>> to > > > > > >>>> their editor behavior, which tread the whole source as > > > > > >>>> if > > > > > >>>> it > > > > > >>>> was > > > > > >>>> at > > > > > >>>> its disposal, while cation should be taken not to modify > > > > > >>>> extra > > > > > >>>> stuff. > > > > > >>>> > > > > > >>>> So maybe just to reject patches that touches lines which > > > > > >>>> are > > > > > >>>> not > > > > > >>>> belong to the patch it-self. > > > > > >>>> > > > > > >>>> Alon > > > > > >>> > > > > > >>> I'm fine with rejecting patches with irrelevant changes. > > > > > >>> Still sometimes you get new files and this repeats > > > > > >>> itself. > > > > > >>> The whole point of a convention is that people keep it. > > > > > >>> Most major IDEs today can use the same formatting XML we > > > > > >>> now > > > > > >>> have. > > > > > >>> So it's really a matter of self-discipline. > > > > > >>> > > > > > >> > > > > > >> OK, I suggest we start rejecting patches with irrelevant > > > > > >> changes, > > > > > >> and > > > > > >> placing these irrelevant changes in their own patch to be > > > > > >> relevant. > > > > > >> > > > > > >> I don't think that we can enforce XML formatting for all > > > > > >> editors > > > > > >> out > > > > > >> there... unless there is a cli utility to reformat before > > > > > >> commit, > > > > > >> or > > > > > >> even do this at gerrit side, we should be somewhat > > > > > >> flexible. > > > > > >> > > > > > >> But these are only my two cents. > > > > > >> > > > > > >> Alon > > > > > > > > > > > > Thanks to Ofri we can now both automate it and monitor it > > > > > > using > > > > > > http://maven-java-formatter-plugin.googlecode.com/svn/site/0.3.1/examples.html. > > > > > > > > > > > > We should be able to add it to the root pom and make sure > > > > > > it > > > > > > won't > > > > > > happen again. > > > > > > > > > > From a quick look this plugin looks like formatted and not > > > > > format-validator, if this is the case it is less useful as we > > > > > can't > > > > > add > > > > > a jenkins job to monitor the expected formatting. > > > > > > > > > > I think that we should clean the formatting (once again...) > > > > > in > > > > > our > > > > > code > > > > > base only after we'll have a valid way to enforce it, > > > > > otherwise > > > > > we > > > > > are > > > > > going to have the same thread every few months (we already > > > > > had > > > > > it > > > > > twice > > > > > or more) and we all hate this massive formatting patches that > > > > > comes > > > > > after the thread... > > > > > > > > > > Livnat > > > > > > > > > > > > > Livnat, > > > > it's easy enough to have a Jenkins job running this plugin. > > > > > > Isn't Jenkins too late in the game for checks of incoming code? > > > IIRC > > > there do exist pre-commit hooks for gerrit that run > > > pylint/pyflakes > > > on > > > python files, that should IMO be the very place to check Java > > > coding > > > style, too. > > > > if the jenkins jobs will run per gerrit patch and not per commit, > > (it's possible to run it via jenkins, like run other jobs per > > patch). > > then it's not too late, and less messy than defining specific hooks > > for it. > > > > Eyal.
Sorry for (very) late response - what would be the price (i.e - build time) for running this plugin while performing "mvn clean install" on ovirt-engine? I know that sometimes people forget to run tests (which is bad) but I do believe (and hope ;) ) they do compile before send patches to gerrit. > > > > Right, we actually had something similar in the past > while .NET was still around... > > > > > > > David > > > > > > > Then git status will tell you if something changed, and if so > > > > you email the changes to patch owner (and nack the patch). We > > > > had similar jobs in WPF era and it's quite simple to handle. > > > > So I see no reason why not to start it now. The sooner we > > > > start, > > > > less noise and unneeded code changes will be added. > > > > > > > > better > > > > _______________________________________________ > > > > Engine-devel mailing list > > > > [email protected] > > > > http://lists.ovirt.org/mailman/listinfo/engine-devel > > > > > > -- > > > > > > David Jaša, RHCE > > > > _______________________________________________ > Engine-devel mailing list > [email protected] > http://lists.ovirt.org/mailman/listinfo/engine-devel > _______________________________________________ Engine-devel mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-devel
