You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by er...@apache.org on 2012/10/30 22:10:10 UTC
svn commit: r1403870 -
/ofbiz/branches/20120329_portletWidget/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java
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
Modified: ofbiz/branches/20120329_portletWidget/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java
URL: http://svn.apache.org/viewvc/ofbiz/branches/20120329_portletWidget/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java?rev=1403870&r1=1403869&r2=1403870&view=diff
==============================================================================
--- ofbiz/branches/20120329_portletWidget/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java (original)
+++ ofbiz/branches/20120329_portletWidget/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java Tue Oct 30 21:10:10 2012
@@ -88,6 +88,7 @@ public class EntitySaxReader implements
protected boolean checkDataOnly = false;
protected boolean doCacheClear = true;
protected boolean disableEeca = false;
+ protected boolean setOtherFieldsToNull = false;
protected List<Object> messageList = null;
protected List<GenericValue> valuesToWrite = new ArrayList<GenericValue>(valuesPerWrite);
@@ -202,6 +203,14 @@ public class EntitySaxReader implements
}
}
+ public boolean getSetOtherFieldsToNull() {
+ return this.setOtherFieldsToNull;
+ }
+
+ public void setSetOtherFieldsToNull(boolean _setOtherFieldsToNull) {
+ this.setOtherFieldsToNull = _setOtherFieldsToNull;
+ }
+
public long parse(String content) throws SAXException, java.io.IOException {
if (content == null) {
Debug.logWarning("content was null, doing nothing", module);
@@ -493,6 +502,12 @@ public class EntitySaxReader implements
this.setCreateDummyFks("true".equalsIgnoreCase(dummyFk.toString()));
}
+ // check if other fields should be set to null
+ CharSequence _setOtherFieldsToNull = attributes.getValue("set-other-fields-to-null");
+ if (_setOtherFieldsToNull != null) {
+ this.setSetOtherFieldsToNull("true".equalsIgnoreCase(_setOtherFieldsToNull.toString()));
+ }
+
return;
}
@@ -575,6 +590,16 @@ public class EntitySaxReader implements
Debug.logWarning(e, "Could not set field " + entityName + "." + name + " to the value " + value, module);
}
}
+ if (this.getSetOtherFieldsToNull()) {
+ ModelEntity currentEntity = currentValue.getModelEntity();
+ for (String fieldName : currentEntity.getAllFieldNames() ){
+ if ( ! ModelEntity.STAMP_FIELD.equals(fieldName) && ! ModelEntity.STAMP_TX_FIELD.equals(fieldName)
+ && ! ModelEntity.CREATE_STAMP_FIELD.equals(fieldName) && ! ModelEntity.CREATE_STAMP_TX_FIELD.equals(fieldName)
+ && currentValue.get(fieldName) == null ) {
+ currentValue.set(fieldName, null);
+ }
+ }
+ }
}
}
}
Re: portletWidget branch review
Posted by Olivier Heintz <ho...@nereide.biz>.
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, erwan@apache.org wrote:
>
Re: svn commit: r1403870 - /ofbiz/branches/20120329_portletWidget/framework/entity/src/org/ofbiz/entit
y/util/EntitySaxReader.java
Posted by Olivier Heintz <ho...@nereide.biz>.
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" <pa...@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
>>>>>>>>
Re: svn commit: r1403870 -
/ofbiz/branches/20120329_portletWidget/framework/entity/src/org/ofbiz/entit
y/util/EntitySaxReader.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
From: "Paul Foxworthy" <pa...@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.
Re: svn commit: r1403870 -
/ofbiz/branches/20120329_portletWidget/framework/entity/src/org/ofbiz/entit
y/util/EntitySaxReader.java
Posted by Paul Foxworthy <pa...@cohsoft.com.au>.
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
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_portletWidget/framework/entity/src/org/ofbiz/entit
y/util/EntitySaxReader.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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" <ja...@hotwaxmedia.com>
To: <de...@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.
>
>
Re: svn commit: r1403870 - /ofbiz/branches/20120329_portletWidget/framework/entity/src/org/ofbiz/entit y/util/EntitySaxReader.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
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_portletWidget/framework/entity/src/org/ofbiz/entit
y/util/EntitySaxReader.java
Posted by Paul Foxworthy <pa...@cohsoft.com.au>.
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_portletWidget/framework/entity/src/org/ofbiz/entit
y/util/EntitySaxReader.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
From: "Jacopo Cappellato" <ja...@hotwaxmedia.com>
> 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" <ja...@hotwaxmedia.com>
>>> 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@apache.org 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_portletWidget/framework/entity/src/org/ofbiz/entit y/util/EntitySaxReader.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
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" <ja...@hotwaxmedia.com>
>> 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@apache.org 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_portletWidget/framework/entity/src/org/ofbiz/entit
y/util/EntitySaxReader.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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" <ja...@hotwaxmedia.com>
> 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@apache.org 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_portletWidget/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
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@apache.org 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_portletWidget/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
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@apache.org 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