Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacopo Cappellato]

On Thu, Mar 30, 2017 at 2:16 PM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:

> [...] Jacopo asked me to prove I was right, it's not the same.


I have never asked you to prove you were right; all I have asked is:

"to revert the changes in which you have changed the functional behavior of
the system without testing OR test the new behavior and confirm it is
working fine"


> [...] Like if I was in a trial, where Jacopo, Scott, Taher and you were
> accusing me.


I suggest you to try not being so defensive and do your best to accept
criticism to your work not as it was criticism to you as a person.
Please also be more respectful to the other committers, and to the people
reading your messages, who are spending time trying to help and protect the
code: accusing them to be an "elite", accusing them to practice FUD (which
is a strong statement for which I would expect your apologies) is really
killing the collaboration.

Jacopo

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacques Le Roux]

Le 30/03/2017 à 14:16, Jacques Le Roux a écrit :
It still happens that I prefer to submit a patch and there are cases in Jira. https://issues.apache.org/jira/issues/?filter=12340482 Not all 
attachments are patches, but even if it's only a half+, it's still around 100 cases

Sorry, this is a personal filter I can only view, use rather 
https://issues.apache.org/jira/issues/?filter=12340487

Jacques

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacques Le Roux]

Hi Michael,

It will be my last answer, and I'll try to be positive, just using facts, else 
we will never end.


Le 29/03/2017 à 11:23, Michael Brohl a écrit :

Hi Jacques,

Am 28.03.17 um 05:47 schrieb Jacques Le Roux:

Le 25/03/2017 à 13:21, Michael Brohl a écrit :

+1

The lack of code documentation is not a free ticket to just change the code 
behaviour without proper analysis.


It's not because the swallowed exceptions where not documented that I decided to catch them. I rather proposed later to at least document them; if 
we had fear (ie no proofs) that it was going to break the flow.

What make you think that I did not a proper analysis? Actually it was a fast 
analysis based on 2 principles:
1) Exceptions should be catched because they fail fast. It's then easier to analyse a stack error. 
https://www.owasp.org/index.php/Exception_handling_techniques#Swallowing_Exceptions. You may prefer 
https://www.google.com/search?q=swallowing+exceptions

And yes, I did a lot of OFBiz log analysis, where stack errors are very 
important. And no, most of the errors I had to analyse were not mine!
2) Returning an error from a service does not guarantee that all cases are 
covered, notably exceptions



These are just general rules and patterns and do not prove that a proper 
analysis for this special case was made.
I did a proper analysis based on these patterns and rules. Jacopo asked me to prove I was right, it's not the same. I got into much details then 
applying the ideas I had initially. Like if I was in a trial, where Jacopo, Scott, Taher and you were accusing me.

Now how could I prove that I previously did a proper analysis? It was in my 
brain, hard to say.
Let me ask you another question. In which cases is it good to swallow an exception? I believe there are very rare cases, if any. And having a 
swallowed exception is a smell for refactoring.
I must admit I did not then dig as much as I did to convince Jacopo I was not derailing the flow. But that's what I had in mind. It was not a shoot 
in the dark as you seem to think.




I did not say that it was a shot in the dark but that it needs proper analysis. You admit for yourself that you did it after Jacopo asked you to 
revert the commit and not before you committed. That's what worries me.

Actually it's very different when you are thinking and when you are 
writing/doing.




The right process should be

1. discuss

2. provide a patch

3. let others review/comment

4. decide

5. commit
I don't agree with this process for present changes. It's much, too much bureaucracy in this case, sorry to say. I though agree that there are 
cases where it's necessary, and I sometimes do so.


That's not bureaucracy but collaboration and a principle to assure quality and prevent errors. As I said before, I won't apply this process to any 
single fix or trivial enhancement.

In this case, what would be so difficult to put the patch for your proposal in 
Jira and asked for some opinions from other contributors?
That's something I do sometimes. But wait contributors do that, and here is the situation: 259 issues with patches available waiting 
https://issues.apache.org/jira/issues/?filter=12340473

among 51 bugs https://issues.apache.org/jira/issues/?filter=12333848
So before submitting a patch I always balance if it's a good idea or if I can take the responsibility of directly committing. I also always consider 
the 10" action rule. If I can do it in 10" then I do it right away. Somrtimes I end shaving the yak and would slap myself :/
It still happens that I prefer to submit a patch and there are cases in Jira. https://issues.apache.org/jira/issues/?filter=12340482 Not all 
attachments are patches, but even if it's only a half+, it's still around 100 cases


In https://issues.apache.org/jira/browse/OFBIZ-9123 you agreed that this woul be the better approach by stating " I concur with Michael's opinion, 
notably the well stated 4 points process."



But, if you read all the comment https://s.apache.org/4HFD I then did a review 
and agree it was OK. Here is another sentence is the same comment
>
For me this did not need to be reverted and been postponed. Later, after Pierre's comment on FTL templates, I asked Jinghai twice to follow the 
structure we "recently" (then) adopted. I guess he missed the 1st and did quickly on the 2nd.


It is really dangerous to easily change code like this.

Why? Please explain and prove your allegations...


It's obvious that changing program logic without proper analysis is dangerous, 
especially in a complex system like OFBiz, isn't it?
I agree OFBiz is complex but this was not complex, people feared it was complex, it was not. "You shall not swallow exceptions" could be the mantra of 
the week.
Jacques, please be not so hasty with committing stuff. 

I don't commit stuff hastily. I commit a lot, but not hastily. I make errors, 
who does not?


You might call it like you want, my impression is that slowing down a bit 

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacques Le Roux]

Le 29/03/2017 à 15:27, Jacques Le Roux a écrit :

It will more clear there where this is at least some limited ways of formatting 
(code, etc.)


Jacques



Typo
It will more clear there where there is at least some limited ways of 
formatting (code, etc.)

Jacques


:D still a word missing
It will BE more clear there, where there is at least some limited ways of 
formatting (code, etc.)

Jacques

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacques Le Roux]

It will more clear there where this is at least some limited ways of formatting 
(code, etc.)


Jacques



Typo
It will more clear there where there is at least some limited ways of 
formatting (code, etc.)

Jacques

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacques Le Roux]

Le 27/03/2017 à 10:01, Jacopo Cappellato a écrit :

On Sat, Mar 25, 2017 at 8:23 AM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:


[...]
Now feel free to revert my commit if you still think it's a bad thing, but
sincerely I'm not convinced! If you do so I'll then at least add comments
to explain the situation...


The analysis and tests that you did in this email to reply to my request
are actually what I was asking for that is (quoting myself):

"[...] to revert the changes in which you have changed the functional
behavior of the system without testing OR test the new behavior and confirm
it is working fine."

So this is the right direction: please similarly complete your analysis and
testing on any other code modified/affected by your commits or revert the
functional changes that you can't analyze or test.

Thanks

Jacopo


To keep things clear I'll answer you an everybody in the related Jira "Fix Default 
or Empty Catch block in Java files" OFBIZ-8341
I will explain each commit in the Jira for everybody interested.
It will more clear there where this is at least some limited ways of formatting 
(code, etc.)

Jacques

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Michael Brohl]

Hi Jacques,

Am 28.03.17 um 05:47 schrieb Jacques Le Roux:

Le 25/03/2017 à 13:21, Michael Brohl a écrit :

+1

The lack of code documentation is not a free ticket to just change 
the code behaviour without proper analysis.


It's not because the swallowed exceptions where not documented that I 
decided to catch them. I rather proposed later to at least document 
them; if we had fear (ie no proofs) that it was going to break the flow.
What make you think that I did not a proper analysis? Actually it was 
a fast analysis based on 2 principles:
1) Exceptions should be catched because they fail fast. It's then 
easier to analyse a stack error. 
https://www.owasp.org/index.php/Exception_handling_techniques#Swallowing_Exceptions. 
You may prefer https://www.google.com/search?q=swallowing+exceptions
And yes, I did a lot of OFBiz log analysis, where stack errors are 
very important. And no, most of the errors I had to analyse were not 
mine!
2) Returning an error from a service does not guarantee that all cases 
are covered, notably exceptions




These are just general rules and patterns and do not prove that a proper 
analysis for this special case was made.


I must admit I did not then dig as much as I did to convince Jacopo I 
was not derailing the flow. But that's what I had in mind. It was not 
a shoot in the dark as you seem to think.




I did not say that it was a shot in the dark but that it needs proper 
analysis. You admit for yourself that you did it after Jacopo asked you 
to revert the commit and not before you committed. That's what worries me.




The right process should be

1. discuss

2. provide a patch

3. let others review/comment

4. decide

5. commit
I don't agree with this process for present changes. It's much, too 
much bureaucracy in this case, sorry to say. I though agree that there 
are cases where it's necessary, and I sometimes do so.


That's not bureaucracy but collaboration and a principle to assure 
quality and prevent errors. As I said before, I won't apply this process 
to any single fix or trivial enhancement.
In this case, what would be so difficult to put the patch for your 
proposal in Jira and asked for some opinions from other contributors?


In https://issues.apache.org/jira/browse/OFBIZ-9123 you agreed that this 
woul be the better approach by stating " I concur with Michael's 
opinion, notably the well stated 4 points process."




It is really dangerous to easily change code like this.

Why? Please explain and prove your allegations...


It's obvious that changing program logic without proper analysis is 
dangerous, especially in a complex system like OFBiz, isn't it?


Jacques, please be not so hasty with committing stuff. 
I don't commit stuff hastily. I commit a lot, but not hastily. I make 
errors, who does not?


You might call it like you want, my impression is that slowing down a 
bit and collaborating more by providing a patch instead of directly 
commiting your work might reduce the number of errors and save us a lot 
of time discussing and reverting afterwards.



We have had a lot of similar cases with reverts,

"A lot of similar cases with reverts"? Really? Which ones?


Just search for "revert/reverted/reverts" in the commits mailing list 
and you'll find them.


committing half done solutions and such lately. 

"half done solutions and such", have you examples?


We had several disccussions about them, the encryption issue and the 
addition of the PriCat component come to mind. There were some others 
but I won't spent time to dig them up again.


And please be aware that others might not have so much time to follow 
every commit in detail, analyze and comment promptly.
I don't ask anybody to follow the pace I have currently the chance to 
have. But if I follow your comment, then we would use RTC, for now 
it's CTR. And with RTC, OFBiz evolution, which is not currently 
brilliant,  would begin to stale.


In my opinion, the evolution should be towards quality, ease of 
maintenance and robustness, not about commiting lots of stuff in a short 
amount of time. A well balanced use of RTC and CTR will help all of us.




It really worries me because we lose quality and it's not easy to 
detect errors 
It never easy to detect errors. It needs a lot of work. Do you suggest 
that I put errors in code by negligence? Have a look at what I do, and 
you will be convinced on the contrary. I track errors as much as I can 
and I help others to fix them when they are not mine.


I did not say any of this nor do I suggest it.

and changed functionality in such a complex project. 
I repeat, again, I did not change any functionalities, hence the "No 
functional change". Prove the contrary...
And don't rely too much on the tests as we don't have such a high 
test coverage.

I don't rely only on tests, but yes I also rely on them, who does not?


Thanks for some more patience,
That I can understand, but not all the FUD above, and not going to RTC 
because of fear. If 

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacques Le Roux]

Le 25/03/2017 à 13:21, Michael Brohl a écrit :

+1

The lack of code documentation is not a free ticket to just change the code 
behaviour without proper analysis.


It's not because the swallowed exceptions where not documented that I decided to catch them. I rather proposed later to at least document them; if we 
had fear (ie no proofs) that it was going to break the flow.

What make you think that I did not a proper analysis? Actually it was a fast 
analysis based on 2 principles:
1) Exceptions should be catched because they fail fast. It's then easier to analyse a stack error. 
https://www.owasp.org/index.php/Exception_handling_techniques#Swallowing_Exceptions. You may prefer https://www.google.com/search?q=swallowing+exceptions

And yes, I did a lot of OFBiz log analysis, where stack errors are very 
important. And no, most of the errors I had to analyse were not mine!
2) Returning an error from a service does not guarantee that all cases are 
covered, notably exceptions

I must admit I did not then dig as much as I did to convince Jacopo I was not derailing the flow. But that's what I had in mind. It was not a shoot in 
the dark as you seem to think.




The right process should be

1. discuss

2. provide a patch

3. let others review/comment

4. decide

5. commit
I don't agree with this process for present changes. It's much, too much bureaucracy in this case, sorry to say. I though agree that there are cases 
where it's necessary, and I sometimes do so.


It is really dangerous to easily change code like this.

Why? Please explain and prove your allegations...


Jacques, please be not so hasty with committing stuff. 

I don't commit stuff hastily. I commit a lot, but not hastily. I make errors, 
who does not?

We have had a lot of similar cases with reverts,

"A lot of similar cases with reverts"? Really? Which ones?
committing half done solutions and such lately. 

"half done solutions and such", have you examples?

And please be aware that others might not have so much time to follow every 
commit in detail, analyze and comment promptly.
I don't ask anybody to follow the pace I have currently the chance to have. But if I follow your comment, then we would use RTC, for now it's CTR. And 
with RTC, OFBiz evolution, which is not currently brilliant,  would begin to stale.


It really worries me because we lose quality and it's not easy to detect errors 
It never easy to detect errors. It needs a lot of work. Do you suggest that I put errors in code by negligence? Have a look at what I do, and you will 
be convinced on the contrary. I track errors as much as I can and I help others to fix them when they are not mine.
and changed functionality in such a complex project. 

I repeat, again, I did not change any functionalities, hence the "No functional 
change". Prove the contrary...

And don't rely too much on the tests as we don't have such a high test coverage.

I don't rely only on tests, but yes I also rely on them, who does not?


Thanks for some more patience,
That I can understand, but not all the FUD above, and not going to RTC because of fear. If you have something to say, please comment the code and 
detail the problems you see, then we can discuss...


Jacques


Michael


Am 24.03.17 um 14:13 schrieb Jacopo Cappellato:

On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:


[...]
If we (both and All) agree on collaborating to document on purpose
swallowed exceptions, even when you are not directly concerned, then I
agree to revert my changes, deal?


We are not negotiating: I have simply asked you to revert the changes in
which you have changed the functional behavior of the system without
testing OR test the new behavior and confirm it is working fine.

In general I like the effort of improving this old code containing
swallowed exceptions by providing more comments, documentation etc... or
completely refactoring it; but this has to be done with proper testing.

I hope this clarifies my request.

Jacopo

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacopo Cappellato]

On Sat, Mar 25, 2017 at 8:23 AM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:

> [...]
> Now feel free to revert my commit if you still think it's a bad thing, but
> sincerely I'm not convinced! If you do so I'll then at least add comments
> to explain the situation...


The analysis and tests that you did in this email to reply to my request
are actually what I was asking for that is (quoting myself):

"[...] to revert the changes in which you have changed the functional
behavior of the system without testing OR test the new behavior and confirm
it is working fine."

So this is the right direction: please similarly complete your analysis and
testing on any other code modified/affected by your commits or revert the
functional changes that you can't analyze or test.

Thanks

Jacopo

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacopo Cappellato]

On Mon, Mar 27, 2017 at 10:24 AM, Michael Brohl 
wrote:

> Hi Jacopo,
>
> I'm not for a general change to Review-Then-Commit but pleading for a more
> sensible way to handle these things as a responsible committer.
>
> As a guideline: if in doubt, always provide a patch and describe what the
> change should do or fix and let others review.
>
> Of course not for trivial changes or fixes. But when it comes to new
> modules, significantly changing the behaviour of the business logic etc. I
> think it's better to apply Review-Then-Commit.
>
> And of course, if others want a commit to be reverted, we should do it
> first and then discuss.
>
> Regards,
>
> Michael
>

I totally agree with all you wrote Michael.

Jacopo

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Michael Brohl]

Hi Jacopo,

I'm not for a general change to Review-Then-Commit but pleading for a 
more sensible way to handle these things as a responsible committer.


As a guideline: if in doubt, always provide a patch and describe what 
the change should do or fix and let others review.


Of course not for trivial changes or fixes. But when it comes to new 
modules, significantly changing the behaviour of the business logic etc. 
I think it's better to apply Review-Then-Commit.


And of course, if others want a commit to be reverted, we should do it 
first and then discuss.


Regards,

Michael


Am 27.03.17 um 10:07 schrieb Jacopo Cappellato:

On Sat, Mar 25, 2017 at 1:21 PM, Michael Brohl 
wrote:


+1

The lack of code documentation is not a free ticket to just change the
code behaviour without proper analysis.

The right process should be

1. discuss

2. provide a patch

3. let others review/comment

4. decide

5. commit

It is really dangerous to easily change code like this.

Jacques, please be not so hasty with committing stuff. We have had a lot
of similar cases with reverts, committing half done solutions and such
lately. And please be aware that others might not have so much time to
follow every commit in detail, analyze and comment promptly.

It really worries me because we lose quality and it's not easy to detect
errors and changed functionality in such a complex project. And don't rely
too much on the tests as we don't have such a high test coverage.

Thanks for some more patience,

Michael



I am fine with continuing with the Commit-Then-Review approach (and
leverage Review-ThenCommit approach only when more appropriate) provided
that the committer is willing to accept (by reverting or performing further
work as requested by the reviewers) negative reviews. What we should really
avoid is committers dragging their feet, pushing back reviews, trying to
argue and negotiate to keep their code as committed.

Jacopo






smime.p7s
Description: S/MIME Cryptographic Signature

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacopo Cappellato]

On Sat, Mar 25, 2017 at 1:21 PM, Michael Brohl 
wrote:

> +1
>
> The lack of code documentation is not a free ticket to just change the
> code behaviour without proper analysis.
>
> The right process should be
>
> 1. discuss
>
> 2. provide a patch
>
> 3. let others review/comment
>
> 4. decide
>
> 5. commit
>
> It is really dangerous to easily change code like this.
>
> Jacques, please be not so hasty with committing stuff. We have had a lot
> of similar cases with reverts, committing half done solutions and such
> lately. And please be aware that others might not have so much time to
> follow every commit in detail, analyze and comment promptly.
>
> It really worries me because we lose quality and it's not easy to detect
> errors and changed functionality in such a complex project. And don't rely
> too much on the tests as we don't have such a high test coverage.
>
> Thanks for some more patience,
>
> Michael
>
>
I am fine with continuing with the Commit-Then-Review approach (and
leverage Review-ThenCommit approach only when more appropriate) provided
that the committer is willing to accept (by reverting or performing further
work as requested by the reviewers) negative reviews. What we should really
avoid is committers dragging their feet, pushing back reviews, trying to
argue and negotiate to keep their code as committed.

Jacopo

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Taher Alkhateeb]

I think we should start a discussion about these types of commits. I have
concerns and problems with the way commits are getting done at the moment.
Meanwhile, I also agree with Jacopo on the need for a thorough review
instead of these _bulk_ commits without looking carefully at all possible
affected locations in the code base. I mean even with extreme care we
sometimes catch deeply hidden bugs.

On Sat, Mar 25, 2017 at 3:21 PM, Michael Brohl 
wrote:

> +1
>
> The lack of code documentation is not a free ticket to just change the
> code behaviour without proper analysis.
>
> The right process should be
>
> 1. discuss
>
> 2. provide a patch
>
> 3. let others review/comment
>
> 4. decide
>
> 5. commit
>
> It is really dangerous to easily change code like this.
>
> Jacques, please be not so hasty with committing stuff. We have had a lot
> of similar cases with reverts, committing half done solutions and such
> lately. And please be aware that others might not have so much time to
> follow every commit in detail, analyze and comment promptly.
>
> It really worries me because we lose quality and it's not easy to detect
> errors and changed functionality in such a complex project. And don't rely
> too much on the tests as we don't have such a high test coverage.
>
> Thanks for some more patience,
>
> Michael
>
>
> Am 24.03.17 um 14:13 schrieb Jacopo Cappellato:
>
> On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
>> jacques.le.r...@les7arts.com> wrote:
>>
>> [...]
>>> If we (both and All) agree on collaborating to document on purpose
>>> swallowed exceptions, even when you are not directly concerned, then I
>>> agree to revert my changes, deal?
>>>
>>
>> We are not negotiating: I have simply asked you to revert the changes in
>> which you have changed the functional behavior of the system without
>> testing OR test the new behavior and confirm it is working fine.
>>
>> In general I like the effort of improving this old code containing
>> swallowed exceptions by providing more comments, documentation etc... or
>> completely refactoring it; but this has to be done with proper testing.
>>
>> I hope this clarifies my request.
>>
>> Jacopo
>>
>>
>
>

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Pierre Smits]

So you're suggesting to apply RtC in stead op CtR regarding improvement
issues, Michael?

I suggest you start that discussion in a new thread and work towards
consensus and documenting.

Best regards,

Pierre

On Saturday, March 25, 2017, Michael Brohl  wrote:

> +1
>
> The lack of code documentation is not a free ticket to just change the
> code behaviour without proper analysis.
>
> The right process should be
>
> 1. discuss
>
> 2. provide a patch
>
> 3. let others review/comment
>
> 4. decide
>
> 5. commit
>
> It is really dangerous to easily change code like this.
>
> Jacques, please be not so hasty with committing stuff. We have had a lot
> of similar cases with reverts, committing half done solutions and such
> lately. And please be aware that others might not have so much time to
> follow every commit in detail, analyze and comment promptly.
>
> It really worries me because we lose quality and it's not easy to detect
> errors and changed functionality in such a complex project. And don't rely
> too much on the tests as we don't have such a high test coverage.
>
> Thanks for some more patience,
>
> Michael
>
>
> Am 24.03.17 um 14:13 schrieb Jacopo Cappellato:
>
>> On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
>> jacques.le.r...@les7arts.com> wrote:
>>
>> [...]
>>> If we (both and All) agree on collaborating to document on purpose
>>> swallowed exceptions, even when you are not directly concerned, then I
>>> agree to revert my changes, deal?
>>>
>>
>> We are not negotiating: I have simply asked you to revert the changes in
>> which you have changed the functional behavior of the system without
>> testing OR test the new behavior and confirm it is working fine.
>>
>> In general I like the effort of improving this old code containing
>> swallowed exceptions by providing more comments, documentation etc... or
>> completely refactoring it; but this has to be done with proper testing.
>>
>> I hope this clarifies my request.
>>
>> Jacopo
>>
>>
>
>

-- 
Pierre Smits

ORRTIZ.COM 
OFBiz based solutions & services

OFBiz Extensions Marketplace
http://oem.ofbizci.net/oci-2/

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Michael Brohl]

+1

The lack of code documentation is not a free ticket to just change the 
code behaviour without proper analysis.


The right process should be

1. discuss

2. provide a patch

3. let others review/comment

4. decide

5. commit

It is really dangerous to easily change code like this.

Jacques, please be not so hasty with committing stuff. We have had a lot 
of similar cases with reverts, committing half done solutions and such 
lately. And please be aware that others might not have so much time to 
follow every commit in detail, analyze and comment promptly.


It really worries me because we lose quality and it's not easy to detect 
errors and changed functionality in such a complex project. And don't 
rely too much on the tests as we don't have such a high test coverage.


Thanks for some more patience,

Michael


Am 24.03.17 um 14:13 schrieb Jacopo Cappellato:

On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:


[...]
If we (both and All) agree on collaborating to document on purpose
swallowed exceptions, even when you are not directly concerned, then I
agree to revert my changes, deal?


We are not negotiating: I have simply asked you to revert the changes in
which you have changed the functional behavior of the system without
testing OR test the new behavior and confirm it is working fine.

In general I like the effort of improving this old code containing
swallowed exceptions by providing more comments, documentation etc... or
completely refactoring it; but this has to be done with proper testing.

I hope this clarifies my request.

Jacopo






smime.p7s
Description: S/MIME Cryptographic Signature

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacques Le Roux]

Le 24/03/2017 à 14:13, Jacopo Cappellato a écrit :

On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:


[...]
If we (both and All) agree on collaborating to document on purpose
swallowed exceptions, even when you are not directly concerned, then I
agree to revert my changes, deal?


We are not negotiating: I have simply asked you to revert the changes in
which you have changed the functional behavior of the system without
testing OR test the new behavior and confirm it is working fine.

I thought you could have given some information on the documentation to put 
into the swallowed exceptions :/

In general I like the effort of improving this old code containing
swallowed exceptions by providing more comments, documentation etc... or
completely refactoring it; but this has to be done with proper testing.

I hope this clarifies my request.

Jacopo


OK so I'll try to answer to your request.
No need to say that all the tests pass. We know we don't directly cover all services but at least we got that when running all tests (gradlew 
testIntegration):
2017-03-24 14:33:03,864 |main |ServiceDispatcher |T| Sync service [test-dispatcher-Oz3Z0DRojV/updateProductionRunTask] finished in [830] 
milliseconds
2017-03-24 14:33:04,173 |main |ServiceDispatcher |T| Sync service [test-dispatcher-Oz3Z0DRojV/updateProductionRunTask] finished in [162] 
milliseconds

It's part of testProductionRunQuickIssueAndProduce through a call to 
productionRunDeclareAndProduce itself calling updateProductionRunTask

I think we can agree that the successful behaviour can't have been derailed by my changes. It's only when a  GenericEntityException or 
GenericServiceException is raised that it might be have been changed, right? So at least in a normal situation (no exceptions) it's OK.


Now what happens when an exception is raised?
In case of a GenericEntityException 2 expressions are concerned. In case of issues it's better to catch them rather than ignore them. Nothing was done 
before, it was simply ignored. How could let a GenericEntityException swallowed here be better? It's better now IMO, shit happens, better not let it 
hits the fan.


In case of a GenericServiceException the sync call to 
issueProductionRunTaskComponent is concerned. It's a simple-method.
Here are the 2 cases in log when putting a typo in 
issueProductionRunTaskComponent

With exception handled:
2017-03-24 20:29:38,267 |main |ServiceDispatcher |T| [[Sync service failed...- total:0.0,since last(Begin):0.0]] - 
'test-dispatcher-m10o2bxhIR / issueProductionRunTaskComponent'

2017-03-24 20:29:38,267 |main |TransactionUtil   |W| Calling 
transaction setRollbackOnly; this stack trace shows where this is happening:
java.lang.Exception: Service [issueProductionRunTaskComponent] threw an 
unexpected exception/error
at 
org.apache.ofbiz.entity.transaction.TransactionUtil.setRollbackOnly(TransactionUtil.java:361)
 [ofbiz.jar:?]
at 
org.apache.ofbiz.entity.transaction.TransactionUtil.rollback(TransactionUtil.java:302)
 [ofbiz.jar:?]
at 
org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:512) 
[ofbiz.jar:?]
at 
org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:227) 
[ofbiz.jar:?]
at 
org.apache.ofbiz.service.GenericDispatcherFactory$GenericDispatcher.runSync(GenericDispatcherFactory.java:88)
 [ofbiz.jar:?]
at 
org.apache.ofbiz.manufacturing.jobshopmgt.ProductionRunServices.updateProductionRunTask(ProductionRunServices.java:2210)
 [ofbiz.jar:?]
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) 
~[?:1.8.0_121]
at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) 
~[?:1.8.0_121]
at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) 
~[?:1.8.0_121]
at java.lang.reflect.Method.invoke(Unknown Source) ~[?:1.8.0_121]
at 
org.apache.ofbiz.service.engine.StandardJavaEngine.serviceInvoker(StandardJavaEngine.java:100)
 [ofbiz.jar:?]
at 
org.apache.ofbiz.service.engine.StandardJavaEngine.runSync(StandardJavaEngine.java:57)
 [ofbiz.jar:?]
at 
org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:395) 
[ofbiz.jar:?]
at 
org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:227) 
[ofbiz.jar:?]
at 
org.apache.ofbiz.service.GenericDispatcherFactory$GenericDispatcher.runSync(GenericDispatcherFactory.java:88)
 [ofbiz.jar:?]
at 
org.apache.ofbiz.manufacturing.jobshopmgt.ProductionRunServices.productionRunDeclareAndProduce(ProductionRunServices.java:1882)
 [ofbiz.jar:?]
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) 
~[?:1.8.0_121]
at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) 
~[?:1.8.0_121]
at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) 
~[?:1.8.0_121]
at 

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacques Le Roux]

I also agreed, I just expected some help to comment why the exceptions were 
swallowed :/

Jacques


Le 24/03/2017 à 21:09, Scott Gray a écrit :

I agree with Jacopo, these are very functional changes that shouldn't be
made without understanding the workflow.  It's good to clean up the code
but it shouldn't be done blindly.

On 25 March 2017 at 02:13, Jacopo Cappellato <
jacopo.cappell...@hotwaxsystems.com> wrote:


On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:


[...]
If we (both and All) agree on collaborating to document on purpose
swallowed exceptions, even when you are not directly concerned, then I
agree to revert my changes, deal?


We are not negotiating: I have simply asked you to revert the changes in
which you have changed the functional behavior of the system without
testing OR test the new behavior and confirm it is working fine.

In general I like the effort of improving this old code containing
swallowed exceptions by providing more comments, documentation etc... or
completely refactoring it; but this has to be done with proper testing.

I hope this clarifies my request.

Jacopo

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Scott Gray]

I agree with Jacopo, these are very functional changes that shouldn't be
made without understanding the workflow.  It's good to clean up the code
but it shouldn't be done blindly.

On 25 March 2017 at 02:13, Jacopo Cappellato <
jacopo.cappell...@hotwaxsystems.com> wrote:

> On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
> jacques.le.r...@les7arts.com> wrote:
>
> > [...]
> > If we (both and All) agree on collaborating to document on purpose
> > swallowed exceptions, even when you are not directly concerned, then I
> > agree to revert my changes, deal?
>
>
> We are not negotiating: I have simply asked you to revert the changes in
> which you have changed the functional behavior of the system without
> testing OR test the new behavior and confirm it is working fine.
>
> In general I like the effort of improving this old code containing
> swallowed exceptions by providing more comments, documentation etc... or
> completely refactoring it; but this has to be done with proper testing.
>
> I hope this clarifies my request.
>
> Jacopo
>

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacopo Cappellato]

On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:

> [...]
> If we (both and All) agree on collaborating to document on purpose
> swallowed exceptions, even when you are not directly concerned, then I
> agree to revert my changes, deal?


We are not negotiating: I have simply asked you to revert the changes in
which you have changed the functional behavior of the system without
testing OR test the new behavior and confirm it is working fine.

In general I like the effort of improving this old code containing
swallowed exceptions by providing more comments, documentation etc... or
completely refactoring it; but this has to be done with proper testing.

I hope this clarifies my request.

Jacopo

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacques Le Roux]

Hi Jacopo,

Thanks for your review. I understand your concerns but IMO if an exception has a good reason to be swallowed then this reason should be documented by 
a comment in the catch. Else how can we quickly differentiate these cases from plain forgotten swallowed exceptions? Apart of course by deeply 
analysing the concerned code which can be difficult in some cases (I agree not much here).


So I don't want to blindly revert but rather want to handle this case positively. So I think we need to have a look at each case and try to 
collaborate for a better future for OFBiz


--

Let's take ProductionRunServices for a start: https://s.apache.org/bjbA

The 1st hunk concerns updateProductionRunTask()

-} catch (GenericEntityException gee) {
-
-} catch (GenericServiceException gee) {
-
+} catch (GenericEntityException | GenericServiceException e) {
+String errMsg = "Problem calling the 
updateProductionRunTaskStatus service";
+Debug.logError(e, errMsg, module);
+return ServiceUtil.returnError(errMsg);
 }

You put it in at r648703 here is the detail https://s.apache.org/uOWj
Then you improved it at r1043899 by adding

 return ServiceUtil.returnError(ServiceUtil.getErrorMessage(resultService));

But you still let the swallowed exceptions (which BTW looks like a quick C/P, both named gee). If I review the concerned try I can't clearly 
understand why. To me errors in services are handled but not other cases.

--
The 2nd hunk concerns approveRequirement
Here you swallowed the exception (I guess you wrote that before the Apache era) 
and also use a  ServiceUtil.returnError

  return ServiceUtil.returnError(UtilProperties.getMessage(resource, 
"ManufacturingRequirementNotExists", locale));

But we can have 2 cases here
1) no requirement w/o exception, OK
2) a simple entity exception for an unknown reason, swallowed
Why not handling the 2nd case?

The 3rd hunk concerns createProductionRunFromRequirement and is the same case 
than the 2nd

For ShoppingCartEvents and ShoppingCartHelper, it's the same kind of cases than 
the 2nd and 3rd hunks above. That should not be hard to document.

If we (both and All) agree on collaborating to document on purpose swallowed exceptions, even when you are not directly concerned, then I agree to 
revert my changes, deal?


Jacques

Le 23/03/2017 à 09:04, Jacopo Cappellato a écrit :

Forwarding an email that I sent yesterday and seems to be lost in the net.

Jacopo

On Tue, Mar 21, 2017 at 10:29 AM, Jacopo Cappellato <
jacopo.cappell...@hotwaxsystems.com> wrote:


Jacques,

I have some concerns about this and similar changes you are committing in
the attempt to improve exception handling: converting a swallowed exception
into a service/event error is actually introducing a pretty relevant
functional change. Maybe the original code was intended to ignore the
exception and move one with additional business logic: however a bulk
change without a deep study/testing of every specific change you are
introducing is not the right way to go in my opinion. I would recommend to
revert these commits and perform an analysis and testing before introducing
these changes.

Jacopo

On Tue, Mar 21, 2017 at 9:40 AM,  wrote:


Author: jleroux
Date: Tue Mar 21 08:40:07 2017
New Revision: 1787906

URL: http://svn.apache.org/viewvc?rev=1787906=rev
Log:
No functional changes, fixes a bunch of swallowed exceptions

Modified:
 ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/
main/java/org/apache/ofbiz/manufacturing/jobshopmgt/Produ
ctionRunServices.java
 ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
/org/apache/ofbiz/order/shoppingcart/ShoppingCartEvents.java
 ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
/org/apache/ofbiz/order/shoppingcart/ShoppingCartHelper.java

Modified: ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/
main/java/org/apache/ofbiz/manufacturing/jobshopmgt/Produ
ctionRunServices.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/app
lications/manufacturing/src/main/java/org/apache/ofbiz/
manufacturing/jobshopmgt/ProductionRunServices.java?rev=
1787906=1787905=1787906=diff

==
--- ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/
main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRunServices.java
(original)
+++ ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/
main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRunServices.java
Tue Mar 21 08:40:07 2017
@@ -2214,10 

Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacopo Cappellato]

Forwarding an email that I sent yesterday and seems to be lost in the net.

Jacopo

On Tue, Mar 21, 2017 at 10:29 AM, Jacopo Cappellato <
jacopo.cappell...@hotwaxsystems.com> wrote:

> Jacques,
>
> I have some concerns about this and similar changes you are committing in
> the attempt to improve exception handling: converting a swallowed exception
> into a service/event error is actually introducing a pretty relevant
> functional change. Maybe the original code was intended to ignore the
> exception and move one with additional business logic: however a bulk
> change without a deep study/testing of every specific change you are
> introducing is not the right way to go in my opinion. I would recommend to
> revert these commits and perform an analysis and testing before introducing
> these changes.
>
> Jacopo
>
> On Tue, Mar 21, 2017 at 9:40 AM,  wrote:
>
>> Author: jleroux
>> Date: Tue Mar 21 08:40:07 2017
>> New Revision: 1787906
>>
>> URL: http://svn.apache.org/viewvc?rev=1787906=rev
>> Log:
>> No functional changes, fixes a bunch of swallowed exceptions
>>
>> Modified:
>> ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/
>> main/java/org/apache/ofbiz/manufacturing/jobshopmgt/Produ
>> ctionRunServices.java
>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
>> /org/apache/ofbiz/order/shoppingcart/ShoppingCartEvents.java
>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
>> /org/apache/ofbiz/order/shoppingcart/ShoppingCartHelper.java
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/
>> main/java/org/apache/ofbiz/manufacturing/jobshopmgt/Produ
>> ctionRunServices.java
>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/app
>> lications/manufacturing/src/main/java/org/apache/ofbiz/
>> manufacturing/jobshopmgt/ProductionRunServices.java?rev=
>> 1787906=1787905=1787906=diff
>> 
>> ==
>> --- ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/
>> main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRunServices.java
>> (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/
>> main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRunServices.java
>> Tue Mar 21 08:40:07 2017
>> @@ -2214,10 +2214,10 @@ public class ProductionRunServices {
>>  }
>> }
>>  }
>> -} catch (GenericEntityException gee) {
>> -
>> -} catch (GenericServiceException gee) {
>> -
>> +} catch (GenericEntityException |
>> GenericServiceException e) {
>> +String errMsg = "Problem calling the
>> updateProductionRunTaskStatus service";
>> +Debug.logError(e, errMsg, module);
>> +return ServiceUtil.returnError(errMsg);
>>  }
>>  }
>>  }
>> @@ -2264,7 +2264,10 @@ public class ProductionRunServices {
>>  GenericValue requirement = null;
>>  try {
>>  requirement = EntityQuery.use(delegator).fro
>> m("Requirement").where("requirementId", requirementId).queryOne();
>> -} catch (GenericEntityException gee) {
>> +} catch (GenericEntityException e) {
>> +String errMsg = "Problem calling the approveRequirement
>> service";
>> +Debug.logError(e, errMsg, module);
>> +return ServiceUtil.returnError(errMsg);
>>  }
>>
>>  if (requirement == null) {
>> @@ -2295,7 +2298,10 @@ public class ProductionRunServices {
>>  GenericValue requirement = null;
>>  try {
>>  requirement = EntityQuery.use(delegator).fro
>> m("Requirement").where("requirementId", requirementId).queryOne();
>> -} catch (GenericEntityException gee) {
>> +} catch (GenericEntityException e) {
>> +String errMsg = "Problem calling the
>> createProductionRunFromRequirement service";
>> +Debug.logError(e, errMsg, module);
>> +return ServiceUtil.returnError(errMsg);
>>  }
>>  if (requirement == null) {
>>  return 
>> ServiceUtil.returnError(UtilProperties.getMessage(resource,
>> "ManufacturingRequirementNotExists", locale));
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
>> /org/apache/ofbiz/order/shoppingcart/ShoppingCartEvents.java
>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/app
>> lications/order/src/main/java/org/apache/ofbiz/order/shoppin
>> gcart/ShoppingCartEvents.java?rev=1787906=1787905=1787906=diff
>> 
>> ==
>> --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
>> /org/apache/ofbiz/order/shoppingcart/ShoppingCartEvents.java (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
>>