Re: svn commit: r1757991 - in /ofbiz/trunk/applications/manu
Locked
Re: svn commit: r1757991 - in /ofbiz/trunk/applications/manu
 
Administrator
|
Hi Ashish, Harsh,
Please don't let swallowed exceptions in code, there were 2 opportunities here ;) Thanks Le 27/08/2016 à 13:27, [hidden email] 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/manu
|
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, [hidden email] 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/manu
|
Administrator
|
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, [hidden email] 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/manu
|
|
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 <[hidden email]> 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, [hidden email] 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/manu
|
Administrator
|
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 <[hidden email]> > 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, [hidden email] 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/manu
|
|
On Tue, Aug 30, 2016 at 7:19 AM, Jacques Le Roux <
[hidden email]> 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/manu
|
Administrator
|
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/manu
|
Administrator
|
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/manu
|
|
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/manu
|
Administrator
|
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/manu
|
|
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 < [hidden email]> 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/manu
|
Administrator
|
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 < > [hidden email]> 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 >>>>> >>>>> >>>>> >>> |
«
Return to OFBiz - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |