Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

Le 11/09/2017 à 10:34, Scott Gray a écrit : Since this is such a complex problem perhaps we better come up with a best practice since we have so many Helper/Worker/Reader classes in OFBiz and a bit of API consistency goes a long way. If a GenericEntityException is thrown within a

Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

Le 11/09/2017 à 10:28, Taher Alkhateeb a écrit : Quoting Scott: "This is often why we can't just bulk fix the issues reported by static analysis tools, because the fix requires a thought-out solution that's totally dependent on the context of the code in question" So I guess him and I are in

Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

Since this is such a complex problem perhaps we better come up with a best practice since we have so many Helper/Worker/Reader classes in OFBiz and a bit of API consistency goes a long way. If a GenericEntityException is thrown within a Helper/Worker/Reader class method, how should we handle it?

Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

Quoting Scott: "This is often why we can't just bulk fix the issues reported by static analysis tools, because the fix requires a thought-out solution that's totally dependent on the context of the code in question" So I guess him and I are in agreement at least in this point. Furthermore, to my

Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

Le 10/09/2017 à 19:56, Taher Alkhateeb a écrit : Inline On Sun, Sep 10, 2017 at 8:40 PM, Jacques Le Roux wrote: I easily see 3 alternatives: 1. Swallow the exception, like we currently do. This should be forbidden in all cases, I'd veto that!- 2. Log an error

Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

Inline On Sun, Sep 10, 2017 at 8:40 PM, Jacques Le Roux wrote: > I easily see 3 alternatives: > > 1. Swallow the exception, like we currently do. This should be forbidden in > all cases, I'd veto that!- > 2. Log an error and return a wrong result, like it's

Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

I easily see 3 alternatives: 1. Swallow the exception, like we currently do. This should be forbidden in all cases, I'd veto that!- 2. Log an error and return a wrong result, like it's currently done by getShippableTotal() in the same class. That's what I proposed, but instead of returning

Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

What I understand is that the patch is essentially changing the signature of most methods to throw an exception. On a first glance this seems to be putting the code in a worst state because now you're adding complexity for the caller to figure out how to handle these exceptions. What is the

Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

Thanks, Jacques. Regards, Michael Am 08.09.17 um 10:54 schrieb Jacques Le Roux: No worries Michael, I can wait a week more Jacques Le 08/09/2017 à 10:42, Michael Brohl a écrit : I disagree, not on the patch itself but on the time frame you give us to review and think about it. I see no

Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

No worries Michael, I can wait a week more Jacques Le 08/09/2017 à 10:42, Michael Brohl a écrit : I disagree, not on the patch itself but on the time frame you give us to review and think about it. I see no reason to put pressure on this issue. Regards, Michael Am 08.09.17 um 10:02

Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

I disagree, not on the patch itself but on the time frame you give us to review and think about it. I see no reason to put pressure on this issue. Regards, Michael Am 08.09.17 um 10:02 schrieb Jacques Le Roux: If nobody disagree my last comments at  OFBIZ-8341 I will commit the

Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

If nobody disagree my last comments at  OFBIZ-8341 I will commit the "OFBIZ-8341- OrderReadHelper.patch" this weekend Jacques Le 05/09/2017 à 07:31, Jacques Le Roux a écrit : Yes thanks Michael, I agree with Scott about rather throwing an exception Jacques Le 04/09/2017 à 21:28, Michael

Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

Thanks Scott, inline... Le 05/09/2017 à 06:21, Scott Gray a écrit : Definitely changes the behavior, but the behavior wasn't good before (and still isn't good now). If we can't return an accurate order item sub-total then we should be throwing an exception IMO. This is often why we can't just

Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java

Yes thanks Michael, I agree with Scott about rather throwing an exception Jacques Le 04/09/2017 à 21:28, Michael Brohl a écrit : Hi Jacques, I think directly returning the result inside the catch block changes the logic of the code (the adjustments are not added). Please have another