Hi Jacques, Am 28.03.17 um 05:47 schrieb Jacques Le Roux:
Le 25/03/2017 à 13:21, Michael Brohl a écrit :+1The 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.
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.The right process should be 1. discuss 2. provide a patch 3. let others review/comment 4. decide 5. commit
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?
I don't commit stuff hastily. I commit a lot, but not hastily. I make errors, who does not?Jacques, please be not so hasty with committing stuff.
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 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.It really worries me because we lose quality and it's not easy to detect errors
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?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...Thanks for some more patience,
This statement tells me that you are not willing to collaborate. Instead, you call arguments for stability and quality assurance FUD and refuse to the request to revert your commit.
Well, that's sad but everyone has his/her own way to contribute... Michael
JacquesMichael 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 inwhich 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 containingswallowed exceptions by providing more comments, documentation etc... orcompletely 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
