From: "Paul Foxworthy" <p...@cohsoft.com.au> > Hi Jacques and Jacopo, > > Maybe set-absent-fields-to-null? Or even nullify-absent-fields?
nullify-absent-fields: +1, this is where I see the difference with someone whose English is mother tongue ;) > 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. I think, it's that indeed, thanks Paul to clarify Jacques > 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: < > >> dev@.apache > >> > >> 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.
