[Engine-devel] Java code formatting

Wed Jan 2 08:24:23 UTC 2013

----- Original Message -----
> From: "Livnat Peer" <lpeer at redhat.com>
> To: "Doron Fediuck" <dfediuck at redhat.com>
> Cc: "engine-devel" <engine-devel at ovirt.org>
> 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" <alonbl at redhat.com>
> >> To: "Doron Fediuck" <dfediuck at redhat.com>
> >> Cc: "engine-devel" <engine-devel at ovirt.org>
> >> Sent: Tuesday, January 1, 2013 5:05:09 PM
> >> Subject: Re: [Engine-devel] Java code formatting
> >>
> >>
> >>
> >> ----- Original Message -----
> >>> From: "Doron Fediuck" <dfediuck at redhat.com>
> >>> To: "Alon Bar-Lev" <alonbl at redhat.com>
> >>> Cc: "engine-devel" <engine-devel at ovirt.org>
> >>> Sent: Tuesday, January 1, 2013 5:00:20 PM
> >>> Subject: Re: [Engine-devel] Java code formatting
> >>>
> >>>
> >>>
> >>> ----- Original Message -----
> >>>> From: "Alon Bar-Lev" <alonbl at redhat.com>
> >>>> To: "Doron Fediuck" <dfediuck at redhat.com>
> >>>> Cc: "engine-devel" <engine-devel at ovirt.org>
> >>>> Sent: Tuesday, January 1, 2013 4:53:49 PM
> >>>> Subject: Re: [Engine-devel] Java code formatting
> >>>>
> >>>>
> >>>>
> >>>> ----- Original Message -----
> >>>>> From: "Doron Fediuck" <dfediuck at redhat.com>
> >>>>> To: "Alon Bar-Lev" <alonbl at redhat.com>
> >>>>> Cc: "engine-devel" <engine-devel at ovirt.org>
> >>>>> Sent: Tuesday, January 1, 2013 4:48:22 PM
> >>>>> Subject: Re: [Engine-devel] Java code formatting
> >>>>>
> >>>>>
> >>>>>
> >>>>> ----- Original Message -----
> >>>>>> From: "Alon Bar-Lev" <alonbl at redhat.com>
> >>>>>> To: "Doron Fediuck" <dfediuck at redhat.com>
> >>>>>> Cc: "engine-devel" <engine-devel at ovirt.org>
> >>>>>> Sent: Tuesday, January 1, 2013 4:33:20 PM
> >>>>>> Subject: Re: [Engine-devel] Java code formatting
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> ----- Original Message -----
> >>>>>>> From: "Doron Fediuck" <dfediuck at redhat.com>
> >>>>>>> To: "Alon Bar-Lev" <alonbl at redhat.com>
> >>>>>>> Cc: "engine-devel" <engine-devel at ovirt.org>
> >>>>>>> Sent: Tuesday, January 1, 2013 4:28:15 PM
> >>>>>>> Subject: Re: [Engine-devel] Java code formatting
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> ----- Original Message -----
> >>>>>>>> From: "Alon Bar-Lev" <alonbl at redhat.com>
> >>>>>>>> To: "Doron Fediuck" <dfediuck at redhat.com>
> >>>>>>>> Cc: "engine-devel" <engine-devel at ovirt.org>
> >>>>>>>> Sent: Tuesday, January 1, 2013 4:17:18 PM
> >>>>>>>> Subject: Re: [Engine-devel] Java code formatting
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> ----- Original Message -----
> >>>>>>>>> From: "Doron Fediuck" <dfediuck at redhat.com>
> >>>>>>>>> To: "engine-devel" <engine-devel at ovirt.org>
> >>>>>>>>> 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.
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