You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Harsh Vijaywargiya <ha...@hotwaxsystems.com> on 2016/08/29 08:17:42 UTC
Re: svn commit: r1757991 - in /ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manu
facturing: bom/ jobshopmgt/ mrp/ techdata/
Thanks Jacques for review comment. I think in such case it could be
better to uncomment such log statements from Exception block, right? or
we should leave it as is?
Thanks & Regards,
Harsh
On Saturday 27 August 2016 06:19 PM, Jacques Le Roux wrote:
> Hi Ashish, Harsh,
>
> Please don't let swallowed exceptions in code, there were 2
> opportunities here ;)
>
> Thanks
>
>
> Le 27/08/2016 � 13:27, ashish@apache.org a �crit :
>> Author: ashish
>> Date: Sat Aug 27 11:27:47 2016
>> New Revision: 1757991
>>
>> URL: http://svn.apache.org/viewvc?rev=1757991&view=rev
>> Log:
>> Applied patch from jira issue - OFBIZ-7848 - Clean up commented out
>> code in Java source for Manufacturing.
>> Thanks Harsh for the contribution.
>>
>> Modified:
>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRun.java
>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRunServices.java
>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/mrp/MrpServices.java
>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/mrp/ProposedOrder.java
>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/techdata/TechDataServices.java
>>
>> Modified:
>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java?rev=1757991&r1=1757990&r2=1757991&view=diff
>> ==============================================================================
>>
>> ---
>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
>> (original)
>> +++
>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
>> Sat Aug 27 11:27:47 2016
>> @@ -436,7 +433,7 @@ public class BOMNode {
>> this.quantity = calcQuantity;
>> }
>> } catch (GenericServiceException e) {
>> - //Debug.logError(e, "Problem calling the
>> getManufacturingComponents service", module);
>> +
>> }
>> } else {
>> this.quantity =
>> quantity.multiply(quantityMultiplier).multiply(scrapFactor);
>> @@ -576,7 +573,7 @@ public class BOMNode {
>> }
>> }
>> } catch (GenericEntityException e) {
>> - //Debug.logError(e, "Problem calling the
>> getManufacturingComponents service", module);
>> +
>> }
>> }
>> return UtilMisc.toMap("productionRunId", productionRunId,
>> "endDate", endDate);
>>
>
Re: svn commit: r1757991 - in
/ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manu
facturing: bom/ jobshopmgt/ mrp/ techdata/
Posted by Jacques Le Roux <ja...@les7arts.com>.
Yes thanks Pierre, just found it in svn annotations, then understood your answer :/
Jacques
Le 20/03/2017 � 11:22, Pierre Smits a �crit :
> Hey Jacques,
>
> This appears to be the related issue: OFBIZ-7848
>
> Best regards,
>
> Pierre Smits
>
> ORRTIZ.COM <http://www.orrtiz.com>
> OFBiz based solutions & services
>
> OFBiz Extensions Marketplace
> http://oem.ofbizci.net/oci-2/
>
> On Mon, Mar 20, 2017 at 11:19 AM, Jacques Le Roux <
> jacques.le.roux@les7arts.com> wrote:
>
>> What is the situation here, please? Is there a Jira? Should I take care of
>> that?
>>
>> Thanks
>>
>> Jacques
>>
>>
>>
>> Le 01/09/2016 � 15:28, Harsh Vijaywargiya a �crit :
>>
>>> Thanks Jacques,
>>>
>>> Sounds good. I will take care of this suggestion in coming commits.
>>>
>>> Thanks & Regards,
>>> Harsh
>>> On Wednesday 31 August 2016 04:51 PM, Jacques Le Roux wrote:
>>>
>>>> OK I checked, the commented out lines were from pre Apache Era. So
>>>> indeed it was not an easy decision.
>>>>
>>>> For
>>>>
>>>> public void print(List<BOMNode> arr, BigDecimal quantity, int depth,
>>>> boolean excludeWIPs) {
>>>>
>>>> I believe the lines were commented out because it's a recursive method.
>>>> I still believe we should never let exceptions escape. The probability it
>>>> happens is low. Another reason to not let it escape: it should not clutter
>>>> the log but when really needed.
>>>>
>>>> So I simply suggest to add
>>>>
>>>> Debug.logError(e, "Problem calling the " + serviceName + " service
>>>> (called by the createManufacturingOrder service)", module);
>>>>
>>>> there.
>>>>
>>>> Globally here is my take
>>>>
>>>> Index: applications/manufacturing/src/main/java/org/apache/ofbiz/
>>>> manufacturing/bom/BOMNode.java
>>>> ===================================================================
>>>> --- applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
>>>> (revision 1758522)
>>>> +++ applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
>>>> (working copy)
>>>> @@ -292,7 +292,7 @@
>>>> variantProduct =
>>>> variantProducts.get(0);
>>>> }
>>>> } catch (GenericServiceException e) {
>>>> - if (Debug.infoOn())
>>>> Debug.logInfo("Error calling getProductVariant service " + e.getMessage(),
>>>> module);
>>>> + Debug.logError("Error calling
>>>> getProductVariant service " + e.getMessage(), module);
>>>> }
>>>> if (variantProduct != null) {
>>>> newNode = new
>>>> BOMNode(variantProduct, dispatcher, userLogin);
>>>> @@ -433,7 +433,7 @@
>>>> this.quantity = calcQuantity;
>>>> }
>>>> } catch (GenericServiceException e) {
>>>> -
>>>> + Debug.logError(e, "Problem calling the " + serviceName
>>>> + " service (called by the createManufacturingOrder service)", module);
>>>> }
>>>> } else {
>>>> this.quantity = quantity.multiply(quantityMult
>>>> iplier).multiply(scrapFactor);
>>>> @@ -573,7 +573,7 @@
>>>> }
>>>> }
>>>> } catch (GenericEntityException e) {
>>>> -
>>>> + Debug.logError(e, "Problem calling the
>>>> getManufacturingComponents service", module);
>>>> }
>>>> }
>>>> return UtilMisc.toMap("productionRunId", productionRunId,
>>>> "endDate", endDate);
>>>>
>>>> What to you think?
>>>>
>>>> Jacques
>>>>
>>>>
>>>> Le 30/08/2016 � 11:21, Jacques Le Roux a �crit :
>>>>
>>>>> Le 30/08/2016 � 08:29, Jacopo Cappellato a �crit :
>>>>>
>>>>>> Highlighting code that could be improved rather than fixing it is a
>>>>>> good
>>>>>> way to help potential contributors.
>>>>>> However, and I think this is the reason for Scott's remark, you should
>>>>>> not
>>>>>> have addressed your review/request to individual committer/contributor
>>>>>> (if
>>>>>> the defect you have noticed was not introduced by their contribution,
>>>>>> as in
>>>>>> this case).
>>>>>>
>>>>> OK, got the subtle nuance, thanks
>>>>>
>>>>> Jacques
>>>>>
>>>>>
>>>>>
>>>
Re: svn commit: r1757991 - in /ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manu
facturing: bom/ jobshopmgt/ mrp/ techdata/
Posted by Pierre Smits <pi...@gmail.com>.
Hey Jacques,
This appears to be the related issue: OFBIZ-7848
Best regards,
Pierre Smits
ORRTIZ.COM <http://www.orrtiz.com>
OFBiz based solutions & services
OFBiz Extensions Marketplace
http://oem.ofbizci.net/oci-2/
On Mon, Mar 20, 2017 at 11:19 AM, Jacques Le Roux <
jacques.le.roux@les7arts.com> wrote:
> What is the situation here, please? Is there a Jira? Should I take care of
> that?
>
> Thanks
>
> Jacques
>
>
>
> Le 01/09/2016 à 15:28, Harsh Vijaywargiya a écrit :
>
>> Thanks Jacques,
>>
>> Sounds good. I will take care of this suggestion in coming commits.
>>
>> Thanks & Regards,
>> Harsh
>> On Wednesday 31 August 2016 04:51 PM, Jacques Le Roux wrote:
>>
>>> OK I checked, the commented out lines were from pre Apache Era. So
>>> indeed it was not an easy decision.
>>>
>>> For
>>>
>>> public void print(List<BOMNode> arr, BigDecimal quantity, int depth,
>>> boolean excludeWIPs) {
>>>
>>> I believe the lines were commented out because it's a recursive method.
>>> I still believe we should never let exceptions escape. The probability it
>>> happens is low. Another reason to not let it escape: it should not clutter
>>> the log but when really needed.
>>>
>>> So I simply suggest to add
>>>
>>> Debug.logError(e, "Problem calling the " + serviceName + " service
>>> (called by the createManufacturingOrder service)", module);
>>>
>>> there.
>>>
>>> Globally here is my take
>>>
>>> Index: applications/manufacturing/src/main/java/org/apache/ofbiz/
>>> manufacturing/bom/BOMNode.java
>>> ===================================================================
>>> --- applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
>>> (revision 1758522)
>>> +++ applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
>>> (working copy)
>>> @@ -292,7 +292,7 @@
>>> variantProduct =
>>> variantProducts.get(0);
>>> }
>>> } catch (GenericServiceException e) {
>>> - if (Debug.infoOn())
>>> Debug.logInfo("Error calling getProductVariant service " + e.getMessage(),
>>> module);
>>> + Debug.logError("Error calling
>>> getProductVariant service " + e.getMessage(), module);
>>> }
>>> if (variantProduct != null) {
>>> newNode = new
>>> BOMNode(variantProduct, dispatcher, userLogin);
>>> @@ -433,7 +433,7 @@
>>> this.quantity = calcQuantity;
>>> }
>>> } catch (GenericServiceException e) {
>>> -
>>> + Debug.logError(e, "Problem calling the " + serviceName
>>> + " service (called by the createManufacturingOrder service)", module);
>>> }
>>> } else {
>>> this.quantity = quantity.multiply(quantityMult
>>> iplier).multiply(scrapFactor);
>>> @@ -573,7 +573,7 @@
>>> }
>>> }
>>> } catch (GenericEntityException e) {
>>> -
>>> + Debug.logError(e, "Problem calling the
>>> getManufacturingComponents service", module);
>>> }
>>> }
>>> return UtilMisc.toMap("productionRunId", productionRunId,
>>> "endDate", endDate);
>>>
>>> What to you think?
>>>
>>> Jacques
>>>
>>>
>>> Le 30/08/2016 à 11:21, Jacques Le Roux a écrit :
>>>
>>>> Le 30/08/2016 à 08:29, Jacopo Cappellato a écrit :
>>>>
>>>>> Highlighting code that could be improved rather than fixing it is a
>>>>> good
>>>>> way to help potential contributors.
>>>>> However, and I think this is the reason for Scott's remark, you should
>>>>> not
>>>>> have addressed your review/request to individual committer/contributor
>>>>> (if
>>>>> the defect you have noticed was not introduced by their contribution,
>>>>> as in
>>>>> this case).
>>>>>
>>>> OK, got the subtle nuance, thanks
>>>>
>>>> Jacques
>>>>
>>>>
>>>>
>>>
>>
>>
>
Re: svn commit: r1757991 - in
/ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manu
facturing: bom/ jobshopmgt/ mrp/ techdata/
Posted by Jacques Le Roux <ja...@les7arts.com>.
What is the situation here, please? Is there a Jira? Should I take care of that?
Thanks
Jacques
Le 01/09/2016 � 15:28, Harsh Vijaywargiya a �crit :
> Thanks Jacques,
>
> Sounds good. I will take care of this suggestion in coming commits.
>
> Thanks & Regards,
> Harsh
> On Wednesday 31 August 2016 04:51 PM, Jacques Le Roux wrote:
>> OK I checked, the commented out lines were from pre Apache Era. So indeed it was not an easy decision.
>>
>> For
>>
>> public void print(List<BOMNode> arr, BigDecimal quantity, int depth, boolean excludeWIPs) {
>>
>> I believe the lines were commented out because it's a recursive method. I still believe we should never let exceptions escape. The probability it
>> happens is low. Another reason to not let it escape: it should not clutter the log but when really needed.
>>
>> So I simply suggest to add
>>
>> Debug.logError(e, "Problem calling the " + serviceName + " service (called by the createManufacturingOrder service)", module);
>>
>> there.
>>
>> Globally here is my take
>>
>> Index: applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
>> ===================================================================
>> --- applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java (revision 1758522)
>> +++ applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java (working copy)
>> @@ -292,7 +292,7 @@
>> variantProduct = variantProducts.get(0);
>> }
>> } catch (GenericServiceException e) {
>> - if (Debug.infoOn()) Debug.logInfo("Error calling getProductVariant service " + e.getMessage(), module);
>> + Debug.logError("Error calling getProductVariant service " + e.getMessage(), module);
>> }
>> if (variantProduct != null) {
>> newNode = new BOMNode(variantProduct, dispatcher, userLogin);
>> @@ -433,7 +433,7 @@
>> this.quantity = calcQuantity;
>> }
>> } catch (GenericServiceException e) {
>> -
>> + Debug.logError(e, "Problem calling the " + serviceName + " service (called by the createManufacturingOrder service)", module);
>> }
>> } else {
>> this.quantity = quantity.multiply(quantityMultiplier).multiply(scrapFactor);
>> @@ -573,7 +573,7 @@
>> }
>> }
>> } catch (GenericEntityException e) {
>> -
>> + Debug.logError(e, "Problem calling the getManufacturingComponents service", module);
>> }
>> }
>> return UtilMisc.toMap("productionRunId", productionRunId, "endDate", endDate);
>>
>> What to you think?
>>
>> Jacques
>>
>>
>> Le 30/08/2016 � 11:21, Jacques Le Roux a �crit :
>>> Le 30/08/2016 � 08:29, Jacopo Cappellato a �crit :
>>>> Highlighting code that could be improved rather than fixing it is a good
>>>> way to help potential contributors.
>>>> However, and I think this is the reason for Scott's remark, you should not
>>>> have addressed your review/request to individual committer/contributor (if
>>>> the defect you have noticed was not introduced by their contribution, as in
>>>> this case).
>>> OK, got the subtle nuance, thanks
>>>
>>> Jacques
>>>
>>>
>>
>
>
Re: svn commit: r1757991 - in /ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manu
facturing: bom/ jobshopmgt/ mrp/ techdata/
Posted by Harsh Vijaywargiya <ha...@hotwaxsystems.com>.
Thanks Jacques,
Sounds good. I will take care of this suggestion in coming commits.
Thanks & Regards,
Harsh
On Wednesday 31 August 2016 04:51 PM, Jacques Le Roux wrote:
> OK I checked, the commented out lines were from pre Apache Era. So
> indeed it was not an easy decision.
>
> For
>
> public void print(List<BOMNode> arr, BigDecimal quantity, int
> depth, boolean excludeWIPs) {
>
> I believe the lines were commented out because it's a recursive
> method. I still believe we should never let exceptions escape. The
> probability it happens is low. Another reason to not let it escape: it
> should not clutter the log but when really needed.
>
> So I simply suggest to add
>
> Debug.logError(e, "Problem calling the " + serviceName + " service
> (called by the createManufacturingOrder service)", module);
>
> there.
>
> Globally here is my take
>
> Index:
> applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
> ===================================================================
> ---
> applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
> (revision 1758522)
> +++
> applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
> (working copy)
> @@ -292,7 +292,7 @@
> variantProduct =
> variantProducts.get(0);
> }
> } catch (GenericServiceException e) {
> - if (Debug.infoOn())
> Debug.logInfo("Error calling getProductVariant service " +
> e.getMessage(), module);
> + Debug.logError("Error calling
> getProductVariant service " + e.getMessage(), module);
> }
> if (variantProduct != null) {
> newNode = new
> BOMNode(variantProduct, dispatcher, userLogin);
> @@ -433,7 +433,7 @@
> this.quantity = calcQuantity;
> }
> } catch (GenericServiceException e) {
> -
> + Debug.logError(e, "Problem calling the " +
> serviceName + " service (called by the createManufacturingOrder
> service)", module);
> }
> } else {
> this.quantity =
> quantity.multiply(quantityMultiplier).multiply(scrapFactor);
> @@ -573,7 +573,7 @@
> }
> }
> } catch (GenericEntityException e) {
> -
> + Debug.logError(e, "Problem calling the
> getManufacturingComponents service", module);
> }
> }
> return UtilMisc.toMap("productionRunId", productionRunId,
> "endDate", endDate);
>
> What to you think?
>
> Jacques
>
>
> Le 30/08/2016 � 11:21, Jacques Le Roux a �crit :
>> Le 30/08/2016 � 08:29, Jacopo Cappellato a �crit :
>>> Highlighting code that could be improved rather than fixing it is a
>>> good
>>> way to help potential contributors.
>>> However, and I think this is the reason for Scott's remark, you
>>> should not
>>> have addressed your review/request to individual
>>> committer/contributor (if
>>> the defect you have noticed was not introduced by their
>>> contribution, as in
>>> this case).
>> OK, got the subtle nuance, thanks
>>
>> Jacques
>>
>>
>
Re: svn commit: r1757991 - in
/ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manu
facturing: bom/ jobshopmgt/ mrp/ techdata/
Posted by Jacques Le Roux <ja...@les7arts.com>.
OK I checked, the commented out lines were from pre Apache Era. So indeed it was not an easy decision.
For
public void print(List<BOMNode> arr, BigDecimal quantity, int depth, boolean excludeWIPs) {
I believe the lines were commented out because it's a recursive method. I still believe we should never let exceptions escape. The probability it
happens is low. Another reason to not let it escape: it should not clutter the log but when really needed.
So I simply suggest to add
Debug.logError(e, "Problem calling the " + serviceName + " service (called by the createManufacturingOrder service)", module);
there.
Globally here is my take
Index: applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
===================================================================
--- applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java (revision 1758522)
+++ applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java (working copy)
@@ -292,7 +292,7 @@
variantProduct = variantProducts.get(0);
}
} catch (GenericServiceException e) {
- if (Debug.infoOn()) Debug.logInfo("Error calling getProductVariant service " + e.getMessage(), module);
+ Debug.logError("Error calling getProductVariant service " + e.getMessage(), module);
}
if (variantProduct != null) {
newNode = new BOMNode(variantProduct, dispatcher, userLogin);
@@ -433,7 +433,7 @@
this.quantity = calcQuantity;
}
} catch (GenericServiceException e) {
-
+ Debug.logError(e, "Problem calling the " + serviceName + " service (called by the createManufacturingOrder service)", module);
}
} else {
this.quantity = quantity.multiply(quantityMultiplier).multiply(scrapFactor);
@@ -573,7 +573,7 @@
}
}
} catch (GenericEntityException e) {
-
+ Debug.logError(e, "Problem calling the getManufacturingComponents service", module);
}
}
return UtilMisc.toMap("productionRunId", productionRunId, "endDate", endDate);
What to you think?
Jacques
Le 30/08/2016 � 11:21, Jacques Le Roux a �crit :
> Le 30/08/2016 � 08:29, Jacopo Cappellato a �crit :
>> Highlighting code that could be improved rather than fixing it is a good
>> way to help potential contributors.
>> However, and I think this is the reason for Scott's remark, you should not
>> have addressed your review/request to individual committer/contributor (if
>> the defect you have noticed was not introduced by their contribution, as in
>> this case).
> OK, got the subtle nuance, thanks
>
> Jacques
>
>
Re: svn commit: r1757991 - in
/ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manu
facturing: bom/ jobshopmgt/ mrp/ techdata/
Posted by Jacques Le Roux <ja...@les7arts.com>.
Le 30/08/2016 � 08:29, Jacopo Cappellato a �crit :
> Highlighting code that could be improved rather than fixing it is a good
> way to help potential contributors.
> However, and I think this is the reason for Scott's remark, you should not
> have addressed your review/request to individual committer/contributor (if
> the defect you have noticed was not introduced by their contribution, as in
> this case).
OK, got the subtle nuance, thanks
Jacques
Re: svn commit: r1757991 - in /ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manu
facturing: bom/ jobshopmgt/ mrp/ techdata/
Posted by Jacopo Cappellato <ja...@hotwaxsystems.com>.
On Tue, Aug 30, 2016 at 7:19 AM, Jacques Le Roux <
jacques.le.roux@les7arts.com> wrote:
> Mmm, then who should fix it?
>
This is an opportunity for anyone reading your message to contribute.
> Of course I could have fixed it myself, but I thought that by providing an
> advice I'd help more.
Highlighting code that could be improved rather than fixing it is a good
way to help potential contributors.
However, and I think this is the reason for Scott's remark, you should not
have addressed your review/request to individual committer/contributor (if
the defect you have noticed was not introduced by their contribution, as in
this case).
Kind regards,
Jacopo
> As we say
>
> "If you give a man a *fish*, you have fed him for a day. If you teach a
> man to *fish*, you have fed him for a lifetime."
>
> Jacques
>
>
>
Re: svn commit: r1757991 - in
/ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manu
facturing: bom/ jobshopmgt/ mrp/ techdata/
Posted by Jacques Le Roux <ja...@les7arts.com>.
Mmm, then who should fix it?
Of course I could have fixed it myself, but I thought that by providing an advice I'd help more. As we say
"If you give a man a *fish*, you have fed him for a day. If you teach a man to *fish*, you have fed him for a lifetime."
Jacques
Le 29/08/2016 � 23:41, Scott Gray a �crit :
> Keep in mind though that dealing with silent exceptions wasn't in the scope
> of the commit. Code reviews are great but we shouldn't expect committers
> to fix issues they didn't introduce.
>
> On 29 August 2016 at 21:04, Jacques Le Roux <ja...@les7arts.com>
> wrote:
>
>> Hi Harsh,
>>
>> Please refer to what is usually done for that, eg
>>
>> in a service
>>
>> } catch (GenericEntityException e) {
>> Debug.logWarning(e, module);
>> Map<String, String> messageMap = UtilMisc.toMap("errMessage",
>> e.getMessage());
>> errMsg = UtilProperties.getMessage("CommonUiLabels",
>> "CommonDatabaseProblem", messageMap, locale);
>> return ServiceUtil.returnError(errMsg);
>> }
>>
>> in a worker or such
>>
>> } catch (GenericEntityException e) {
>> Debug.logError(e, module);
>> return ServiceUtil.returnError(UtilPr
>> operties.getMessage(resourceError,
>> "AccountingBillingAccountNotFound",
>> UtilMisc.toMap("billingAccountId", billingAccountId),
>> locale));
>> }
>>
>> YMMV, you may find more examples types, though we should have as less as
>> possible such types...
>>
>> Jacques
>>
>>
>>
>>
>> Le 29/08/2016 � 10:17, Harsh Vijaywargiya a �crit :
>>
>>> Thanks Jacques for review comment. I think in such case it could be
>>> better to uncomment such log statements from Exception block, right? or we
>>> should leave it as is?
>>>
>>> Thanks & Regards,
>>> Harsh
>>>
>>>
>>> On Saturday 27 August 2016 06:19 PM, Jacques Le Roux wrote:
>>>
>>>> Hi Ashish, Harsh,
>>>>
>>>> Please don't let swallowed exceptions in code, there were 2
>>>> opportunities here ;)
>>>>
>>>> Thanks
>>>>
>>>>
>>>> Le 27/08/2016 � 13:27, ashish@apache.org a �crit :
>>>>
>>>>> Author: ashish
>>>>> Date: Sat Aug 27 11:27:47 2016
>>>>> New Revision: 1757991
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1757991&view=rev
>>>>> Log:
>>>>> Applied patch from jira issue - OFBIZ-7848 - Clean up commented out
>>>>> code in Java source for Manufacturing.
>>>>> Thanks Harsh for the contribution.
>>>>>
>>>>> Modified:
>>>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>>> apache/ofbiz/manufacturing/bom/BOMNode.java
>>>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>>> apache/ofbiz/manufacturing/jobshopmgt/ProductionRun.java
>>>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>>> apache/ofbiz/manufacturing/jobshopmgt/ProductionRunServices.java
>>>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>>> apache/ofbiz/manufacturing/mrp/MrpServices.java
>>>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>>> apache/ofbiz/manufacturing/mrp/ProposedOrder.java
>>>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>>> apache/ofbiz/manufacturing/techdata/TechDataServices.java
>>>>>
>>>>> Modified: ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>>> apache/ofbiz/manufacturing/bom/BOMNode.java
>>>>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/manufa
>>>>> cturing/src/main/java/org/apache/ofbiz/manufacturing/
>>>>> bom/BOMNode.java?rev=1757991&r1=1757990&r2=1757991&view=diff
>>>>> ============================================================
>>>>> ==================
>>>>> --- ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>>> apache/ofbiz/manufacturing/bom/BOMNode.java (original)
>>>>> +++ ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>>> apache/ofbiz/manufacturing/bom/BOMNode.java Sat Aug 27 11:27:47 2016
>>>>> @@ -436,7 +433,7 @@ public class BOMNode {
>>>>> this.quantity = calcQuantity;
>>>>> }
>>>>> } catch (GenericServiceException e) {
>>>>> - //Debug.logError(e, "Problem calling the
>>>>> getManufacturingComponents service", module);
>>>>> +
>>>>> }
>>>>> } else {
>>>>> this.quantity = quantity.multiply(quantityMult
>>>>> iplier).multiply(scrapFactor);
>>>>> @@ -576,7 +573,7 @@ public class BOMNode {
>>>>> }
>>>>> }
>>>>> } catch (GenericEntityException e) {
>>>>> - //Debug.logError(e, "Problem calling the
>>>>> getManufacturingComponents service", module);
>>>>> +
>>>>> }
>>>>> }
>>>>> return UtilMisc.toMap("productionRunId", productionRunId,
>>>>> "endDate", endDate);
>>>>>
>>>>>
>>>
Re: svn commit: r1757991 - in /ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manu
facturing: bom/ jobshopmgt/ mrp/ techdata/
Posted by Scott Gray <sc...@hotwaxsystems.com>.
Keep in mind though that dealing with silent exceptions wasn't in the scope
of the commit. Code reviews are great but we shouldn't expect committers
to fix issues they didn't introduce.
On 29 August 2016 at 21:04, Jacques Le Roux <ja...@les7arts.com>
wrote:
> Hi Harsh,
>
> Please refer to what is usually done for that, eg
>
> in a service
>
> } catch (GenericEntityException e) {
> Debug.logWarning(e, module);
> Map<String, String> messageMap = UtilMisc.toMap("errMessage",
> e.getMessage());
> errMsg = UtilProperties.getMessage("CommonUiLabels",
> "CommonDatabaseProblem", messageMap, locale);
> return ServiceUtil.returnError(errMsg);
> }
>
> in a worker or such
>
> } catch (GenericEntityException e) {
> Debug.logError(e, module);
> return ServiceUtil.returnError(UtilPr
> operties.getMessage(resourceError,
> "AccountingBillingAccountNotFound",
> UtilMisc.toMap("billingAccountId", billingAccountId),
> locale));
> }
>
> YMMV, you may find more examples types, though we should have as less as
> possible such types...
>
> Jacques
>
>
>
>
> Le 29/08/2016 à 10:17, Harsh Vijaywargiya a écrit :
>
>> Thanks Jacques for review comment. I think in such case it could be
>> better to uncomment such log statements from Exception block, right? or we
>> should leave it as is?
>>
>> Thanks & Regards,
>> Harsh
>>
>>
>> On Saturday 27 August 2016 06:19 PM, Jacques Le Roux wrote:
>>
>>> Hi Ashish, Harsh,
>>>
>>> Please don't let swallowed exceptions in code, there were 2
>>> opportunities here ;)
>>>
>>> Thanks
>>>
>>>
>>> Le 27/08/2016 à 13:27, ashish@apache.org a écrit :
>>>
>>>> Author: ashish
>>>> Date: Sat Aug 27 11:27:47 2016
>>>> New Revision: 1757991
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1757991&view=rev
>>>> Log:
>>>> Applied patch from jira issue - OFBIZ-7848 - Clean up commented out
>>>> code in Java source for Manufacturing.
>>>> Thanks Harsh for the contribution.
>>>>
>>>> Modified:
>>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>> apache/ofbiz/manufacturing/bom/BOMNode.java
>>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>> apache/ofbiz/manufacturing/jobshopmgt/ProductionRun.java
>>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>> apache/ofbiz/manufacturing/jobshopmgt/ProductionRunServices.java
>>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>> apache/ofbiz/manufacturing/mrp/MrpServices.java
>>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>> apache/ofbiz/manufacturing/mrp/ProposedOrder.java
>>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>> apache/ofbiz/manufacturing/techdata/TechDataServices.java
>>>>
>>>> Modified: ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>> apache/ofbiz/manufacturing/bom/BOMNode.java
>>>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/manufa
>>>> cturing/src/main/java/org/apache/ofbiz/manufacturing/
>>>> bom/BOMNode.java?rev=1757991&r1=1757990&r2=1757991&view=diff
>>>> ============================================================
>>>> ==================
>>>> --- ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>> apache/ofbiz/manufacturing/bom/BOMNode.java (original)
>>>> +++ ofbiz/trunk/applications/manufacturing/src/main/java/org/
>>>> apache/ofbiz/manufacturing/bom/BOMNode.java Sat Aug 27 11:27:47 2016
>>>> @@ -436,7 +433,7 @@ public class BOMNode {
>>>> this.quantity = calcQuantity;
>>>> }
>>>> } catch (GenericServiceException e) {
>>>> - //Debug.logError(e, "Problem calling the
>>>> getManufacturingComponents service", module);
>>>> +
>>>> }
>>>> } else {
>>>> this.quantity = quantity.multiply(quantityMult
>>>> iplier).multiply(scrapFactor);
>>>> @@ -576,7 +573,7 @@ public class BOMNode {
>>>> }
>>>> }
>>>> } catch (GenericEntityException e) {
>>>> - //Debug.logError(e, "Problem calling the
>>>> getManufacturingComponents service", module);
>>>> +
>>>> }
>>>> }
>>>> return UtilMisc.toMap("productionRunId", productionRunId,
>>>> "endDate", endDate);
>>>>
>>>>
>>>
>>
>>
>
Re: svn commit: r1757991 - in
/ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manu
facturing: bom/ jobshopmgt/ mrp/ techdata/
Posted by Jacques Le Roux <ja...@les7arts.com>.
Hi Harsh,
Please refer to what is usually done for that, eg
in a service
} catch (GenericEntityException e) {
Debug.logWarning(e, module);
Map<String, String> messageMap = UtilMisc.toMap("errMessage", e.getMessage());
errMsg = UtilProperties.getMessage("CommonUiLabels", "CommonDatabaseProblem", messageMap, locale);
return ServiceUtil.returnError(errMsg);
}
in a worker or such
} catch (GenericEntityException e) {
Debug.logError(e, module);
return ServiceUtil.returnError(UtilProperties.getMessage(resourceError,
"AccountingBillingAccountNotFound",
UtilMisc.toMap("billingAccountId", billingAccountId), locale));
}
YMMV, you may find more examples types, though we should have as less as possible such types...
Jacques
Le 29/08/2016 � 10:17, Harsh Vijaywargiya a �crit :
> Thanks Jacques for review comment. I think in such case it could be better to uncomment such log statements from Exception block, right? or we
> should leave it as is?
>
> Thanks & Regards,
> Harsh
>
>
> On Saturday 27 August 2016 06:19 PM, Jacques Le Roux wrote:
>> Hi Ashish, Harsh,
>>
>> Please don't let swallowed exceptions in code, there were 2 opportunities here ;)
>>
>> Thanks
>>
>>
>> Le 27/08/2016 � 13:27, ashish@apache.org a �crit :
>>> Author: ashish
>>> Date: Sat Aug 27 11:27:47 2016
>>> New Revision: 1757991
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1757991&view=rev
>>> Log:
>>> Applied patch from jira issue - OFBIZ-7848 - Clean up commented out code in Java source for Manufacturing.
>>> Thanks Harsh for the contribution.
>>>
>>> Modified:
>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRun.java
>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRunServices.java
>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/mrp/MrpServices.java
>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/mrp/ProposedOrder.java
>>> ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/techdata/TechDataServices.java
>>>
>>> Modified: ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java?rev=1757991&r1=1757990&r2=1757991&view=diff
>>> ==============================================================================
>>> --- ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java (original)
>>> +++ ofbiz/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java Sat Aug 27 11:27:47 2016
>>> @@ -436,7 +433,7 @@ public class BOMNode {
>>> this.quantity = calcQuantity;
>>> }
>>> } catch (GenericServiceException e) {
>>> - //Debug.logError(e, "Problem calling the getManufacturingComponents service", module);
>>> +
>>> }
>>> } else {
>>> this.quantity = quantity.multiply(quantityMultiplier).multiply(scrapFactor);
>>> @@ -576,7 +573,7 @@ public class BOMNode {
>>> }
>>> }
>>> } catch (GenericEntityException e) {
>>> - //Debug.logError(e, "Problem calling the getManufacturingComponents service", module);
>>> +
>>> }
>>> }
>>> return UtilMisc.toMap("productionRunId", productionRunId, "endDate", endDate);
>>>
>>
>
>