Re: svn commit: r1403870 - /ofbiz/branches/20120329_portl
Locked
Re: svn commit: r1403870 - /ofbiz/branches/20120329_portl
 
|
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, [hidden email] 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 |
Re: svn commit: r1403870 - /ofbiz/branches/20120329_portl
|
|
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, [hidden email] 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 > |
Re: svn commit: r1403870 - /ofbiz/branches/20120329_portl
Administrator
|
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" <[hidden email]> > 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, [hidden email] 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 >> > > |
Re: svn commit: r1403870 - /ofbiz/branches/20120329_portl
|
|
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" <[hidden email]> >> 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, [hidden email] 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 >>> >> >> |
Re: svn commit: r1403870 - /ofbiz/branches/20120329_portl
|
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
--
Coherent Software Australia Pty Ltd http://www.coherentsoftware.com.au/ Bonsai ERP, the all-inclusive ERP system http://www.bonsaierp.com.au/ |
Re: svn commit: r1403870 - /ofbiz/branches/20120329_portl
|
Administrator
|
In reply to this post by Jacopo Cappellato-4
From: "Jacopo Cappellato" <[hidden email]>
> 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. Yes, I was just discussing about adding this to trunk (my +1) and then no needs to have it in 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. I did not discuss this, as I did not review Erwan's work on the branch at all. I agree that there should be only stricly portletWidget work in this branch. I can't say +1 or -1 without review, so would be a 0 for me. Jacques > > 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" <[hidden email]> >>> 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, [hidden email] 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 >>>> >>> >>> > > |
Re: svn commit: r1403870 - /ofbiz/branches/20120329_portl
|
|
In reply to this post by Paul Foxworthy
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. |
Re: svn commit: r1403870 - /ofbiz/branches/20120329_portl
|
Administrator
|
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" <[hidden email]> To: <[hidden email]> 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. > > |
Re: svn commit: r1403870 - /ofbiz/branches/20120329_portl
|
|
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
--
Coherent Software Australia Pty Ltd http://www.coherentsoftware.com.au/ Bonsai ERP, the all-inclusive ERP system http://www.bonsaierp.com.au/ |
Re: svn commit: r1403870 - /ofbiz/branches/20120329_portl
|
Administrator
|
From: "Paul Foxworthy" <[hidden email]>
> 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. |
Re: svn commit: r1403870 - /ofbiz/branches/20120329_portl
|
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" <[hidden email]> >> 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 >>>>>>>> |
Re: portletWidget branch review
|
|
In reply to this post by Jacopo Cappellato-4
Le 17/11/2012 07:54, Jacopo Cappellato a écrit :
> 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. Since this branch was created we tried to operate only by Jira to give visibility to each update and to detail each modification (to help more committer to be involve in the branch review) . It's true that I forgot to link this Jira to portletWidget when I argue him to integrated it. If an other ways exist to propose a modification and having committer involve in review, give me the point to do. I and Erwan, we have tried to simplify committers review and to do a lot of tests and present a completely finished realization. I don't say we have done no mistake, but we have tried to add maximum of visiblity or help : In portletWidget there are some framework enhancement expose in mailing-list before and for each main point: - a use case in example, and sometime one in Party - developer help is written to explain how to use and what is the best practice It's not easy to maintain branch update and need some review or test, for most of correction after merge we have tried to give clear commit comment. I understand it's not easy to do contributor review, but you should trust in the efforts being made to meet your requests to have a better ofbiz. Please, do not forget, contributors have also (like commiter or PMC) to manage ofbiz bugs on day to day, and so, want a better ofbiz project. Olivier a contributor trying to help community for - better contributions - better ways to contribute > 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, [hidden email] wrote: > |
«
Return to OFBiz - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |