Hi Jacques and Jacopo, Maybe set-absent-fields-to-null? Or even nullify-absent-fields?
I think the idea behind the option is that when you are first importing new data, quite naturally all fields other than those specified in the file will be null. If, however, you're updating existing data, you might want any field not specified in the file to retain its current value, or you might want the contents of the file to specify everything about a record, in other words "take this data and null out the rest". The alternative to set-other-fields-to-null (or whatever else we might call it) would be a huge number of attribute="" in the file. Cheers Paul Foxworthy Jacques Le Roux wrote > Just reviewed (thought Erwan followed our code formatting convention) > > I must say it's a matter to taste for variable names and formatting > But I agree: > * no underscore needed in front of variable name/s, > * Formatting was done to aligne expressions I guess. This is no > recommended by our (aging) conventions (based on an old Sun document, I > think it's time to amend it a bit[1]) but I would not be a fundamentalist > on this point: it does not make reading harder, even easier maybe... > * I have no problem with set-other-fields-to-null but would use > "'set-non-present-fields-to-null" rather then (no pb if it's long, will be > rarely used and then you get the point) > > Not sure I completly understand the Jira description (the requirement it > seems) either. > > Jacques > [1] I don't agree with all but, this article got some good points > http://www.javacodegeeks.com/2012/10/java-coding-conventions-considered-harmful.html?utm_source=feedburner&utm_medium=twitter&utm_campaign=Feed%3A+JavaCodeGeeks+%28Java+Code+Geeks%29 > I would keep: > 1. Line lenght (80 is ridiculous, it remembers me punched cards :D ) > 2. variable names above comments > 3. I agree on 6.3 placement > Opinions? (sorry for sidetracking, I will create a thread if some are > interested....) > > ----- Original Message ----- > From: "Jacopo Cappellato" < > jacopo.cappellato@ > > > To: < > [email protected] > > > Sent: Saturday, November 17, 2012 11:56 AM > Subject: Re: svn commit: r1403870 - > /ofbiz/branches/20120329_portletWidget/framework/entity/src/org/ofbiz/entit > y/util/EntitySaxReader.java > > >> Thank you Paul. >> >> After a cursory review I also see (in very few lines of the >> contribution): >> >> * bad formatting >> * a bad variable name (why _setOtherFieldsToNull rather than >> setOtherFieldsToNull) >> * I am also not sure I like the attribute name set-other-fields-to-null >> (that is btw better than put-other-fields-to-null) >> >> and last of all, frankly speaking, I don't understand the meaning of the >> description in Jira: >> >> "This enhancement is useful when a entity is load by reader (ex: seed) >> and sometime, it could be modify in data file. If a field is change from >> a value to null, currently this modification will not be done in the next >> load. >> For portletWidget, entity PortalPortal have a lot of field with potential >> default value, so sometime, first release use some field and when it's >> reviewed and corrected, some field are changed to null to use the default >> value (to follow best practice)." >> >> Kind regards, >> >> Jacopo >> >> >> On Nov 17, 2012, at 11:22 AM, Paul Foxworthy wrote: >> >>> Hi all, >>> >>> I have no strong opinion on the change itself, which I suppose means I >>> haven't had a use case that would need it. But the commit change >>> description >>> is misleading. In the Jira discussion for OFBIZ-4949 I proposed the name >>> set-other-fields-to-null instead of put-other-field-to-null, and Olivier >>> changed his patch to use that name. If the change is committed to trunk >>> or >>> anywhere else, please fix the description. I have just tweaked the title >>> for >>> OFBIZ-4949. >>> >>> Cheers >>> >>> Paul Foxworthy >>> >>> >>> Jacopo Cappellato-4 wrote >>>> If you agree with me than let's commit to trunk first (if the >>>> objections >>>> from committers are cleared, and I am not sure it is the case with >>>> Scott's >>>> one, even if I didn't review this particular one) and remove it from >>>> the >>>> branch. >>>> But most importantly: are we (and are you) sure that this was the only >>>> patch that was committed to the branch but it is not strictly related >>>> to >>>> the portletWidget work? The fact that I am not sure about it is the >>>> main >>>> motivation for my -1. >>>> >>>> Kind regards, >>>> >>>> Jacopo >>>> >>>> On Nov 17, 2012, at 10:34 AM, Jacques Le Roux wrote: >>>> >>>>> Hi Jacopo, >>>>> >>>>> I understand your formal concerns about being mixed with the branch >>>>> and I >>>>> agree with you. >>>>> >>>>> Apart that, I did not find anything against this patch >>>>> http://ofbiz.markmail.org/search/?q=OFBIZ-4949 >>>>> Only Scoot's comment about using fieldName="" which is cleary a less >>>>> dangerous but also less powerfull solution for the requirement >>>>> >>>>> I don't see it as something dangerous since it would be only used by >>>>> file >>>>> and with a clear intention of the author. Do I miss something? Else >>>>> would >>>>> be a +1 for me to be directly in trunk >>>>> >>>>> Jacques >>>>> >>>>> From: "Jacopo Cappellato" < >>> >>>> jacopo.cappellato@ >>> >>>> > >>>>>> Just to clarify: I understand that this feature is useful for the >>>>>> portletWidget implementation, but it is a *framework* feature that >>>>>> has >>>>>> to be discussed/approved/committed to trunk before the portletWidget >>>>>> code can use it, not vice versa. >>>>>> >>>>>> Jacopo >>>>>> >>>>>> On Nov 17, 2012, at 7:54 AM, Jacopo Cappellato wrote: >>>>>> >>>>>>> Erwan, >>>>>>> >>>>>>> could you please explain why this patch was committed to the >>>>>>> portletWidget branch? There were some objections in Jira and in >>>>>>> general >>>>>>> there was no general approval for the inclusion. Also, it was a >>>>>>> patch >>>>>>> for the trunk, not the branch. >>>>>>> >>>>>>> This is not the way to go, the branch is not the playground of one >>>>>>> committer and we cannot use it as an easy way (a lot of traffic, >>>>>>> less >>>>>>> reviews from committers) to see the code we like committed to trunk. >>>>>>> If >>>>>>> this is the general trend, I am tempted to say that the experiment >>>>>>> of >>>>>>> branches (mostly) used by one committer is failing: branches make >>>>>>> sense >>>>>>> only if a relevant part of the committer group is working on new >>>>>>> stuff, >>>>>>> not just one. >>>>>>> >>>>>>> Kind regards, >>>>>>> >>>>>>> Jacopo >>>>>>> >>>>>>> PS: a message to all: since I am not going to review each and every >>>>>>> commit done on this branch, I am going to vote -1 to the merging of >>>>>>> the >>>>>>> portletWidget branch with the trunk until I will get enough >>>>>>> guarantees >>>>>>> from the people that worked on it that the changes will be only >>>>>>> related >>>>>>> to the original purpose of the branch. >>>>>>> >>>>>>> On Oct 30, 2012, at 10:10 PM, >>> >>>> erwan@ >>> >>>> wrote: >>>>>>> >>>>>>>> Author: erwan >>>>>>>> Date: Tue Oct 30 21:10:10 2012 >>>>>>>> New Revision: 1403870 >>>>>>>> >>>>>>>> URL: http://svn.apache.org/viewvc?rev=1403870&view=rev >>>>>>>> Log: >>>>>>>> Applying a patch from Olivier Heintz on branch OFBIZ-4949 add a new >>>>>>>> attribute for for entity-engine-xml tag, put-other-field-to-null= >>>>>>>> true, if it exist at the beginning data file, all update will put >>>>>>>> to >>>>>>>> null all field not detail in this file >>>>>>>> >>>>>>>> Modified: >>>>>>>> >>>>>>>> ofbiz/branches/20120329_portletWidget/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java >>>>>>> >>>>>> >>>>>> >>> >>> >>> >>> >>> >>> ----- >>> -- >>> Coherent Software Australia Pty Ltd >>> http://www.coherentsoftware.com.au/ >>> >>> Bonsai ERP, the all-inclusive ERP system >>> http://www.bonsaierp.com.au/ >>> >>> -- >>> View this message in context: >>> http://ofbiz.135035.n4.nabble.com/Re-svn-commit-r1403870-ofbiz-branches-20120329-portletWidget-framework-entity-src-org-ofbiz-entity-ua-tp4637684p4637692.html >>> Sent from the OFBiz - Dev mailing list archive at Nabble.com. >> >> ----- -- Coherent Software Australia Pty Ltd http://www.coherentsoftware.com.au/ Bonsai ERP, the all-inclusive ERP system http://www.bonsaierp.com.au/ -- View this message in context: http://ofbiz.135035.n4.nabble.com/Re-svn-commit-r1403870-ofbiz-branches-20120329-portletWidget-framework-entity-src-org-ofbiz-entity-ua-tp4637684p4637716.html Sent from the OFBiz - Dev mailing list archive at Nabble.com.
