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
<<I did not a thorough review, but globally this is very simple and seems well done, so for me it's only lacking documentation. At this stage I'd not
ask to revert.>>
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 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.
I know you prefer RTC over CTR. With 259 issues, among 51 bugs with patches available waiting I don't think RTC would improve this statement, I
believe the contrary.
It's good that committers review commits. And it would be as good if they were
reviewing waiting patches before they become stale...
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.
Roughly
I did 357 commits in the last 3 months (see the stats panel) at
https://lists.apache.org/[email protected]:lte=3M:jleroux
Among them 15 reverts
https://lists.apache.org/[email protected]:lte=3M:jleroux%20reverted
4%, this does not look as terrible stats to me. We have a saying in French which translate to "Only those who do nothing make no mistakes." I don't
want committers, of course including myself, to feel afraid of committing.
But maybe you should ask in a new thread about his point. But please it must simple RTC vs CTR, because having too much rules will not help, but will
blur things.
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.
Please could you explain what you have in mind with "the encryption issue". For
Pricat I already answered above.
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.
Good words, who will be against "quality, ease of maintenance and robustness". I said it already I'm not systematically against RTC, but I'm against
systematic RTC.
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 you have something to say, please comment the code and
detail the problems you see, then we can discuss...
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.
I don't "call argument for stability and quality assurance" FUD. I wanted to say that I'm still waiting your review and details about the issue you
found in my commit.
Well, that's sad but everyone has his/her own way to contribute...
I'm not sad, I'm happy :) Happy committer is happy
As a last word: I know we both want to do good for OFBiz; so I think we should
stop this thread here or with your last answer.
Jacques
Michael
Jacques
Michael
Am 24.03.17 um 14:13 schrieb Jacopo Cappellato:
On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
[email protected]> 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
