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.
