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.cappell...@hotwaxmedia.com> To: <dev@ofbiz.apache.org> 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. > >
