excuse me for my late reply Thank you Jaccopo for review (I will check each point) and Paul to clarify the goal of the modification, it's exactly what I have tried to say. With portlet enhancement I propose, there are a lot field in PortalPortlet with a default value, so best practice is to use field only when something is specifics and so default value is not usable. Ex : formName with portletName as default value So the use case is : 1) first portlet release formName value is not empty 2) after review it's possible to use the portletName as formName, so correction of formName in xxxxForms.xml and nullify formName field in PortalPortlet data file 3) after update correction will be ok only if loading the data file will nullify field in database.
Second point, why have comitted on the portletWidget branch rather than trunk I have argue to Erwan than I was able to easily give use case for PortalPortlet and this modification is needed to multiple test of portletWidget. When I have write the Jira and answer to Paul (and no other remark after) I understand that this change does not generate a problem for someone, but perhaps lacked an real use-case. Le 18/11/2012 09:15, Jacques Le Roux a écrit : > 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 >>>>>>>>
