On 05/20/2012 02:51 AM, Jacopo Cappellato wrote:
On May 20, 2012, at 2:07 AM, Adam Heath wrote:
I have replaced the usage of the static instance of the BeansWrapper with a
newly created object.
You didn't go far enough. There are enough other getDefaultInstance() calls in
our code that need to be fixed.
I'll correct those, because I also need to have a central location to properly
configure the BeansWrapper, based on the ftl breakage.
Thank you Adam, you are completely right abut this and my commit rev. 1340631
should have fixed them all; however I would appreciate as I have made private
and final a couple of fields in FreeMarkerWorker.
I hope this will make your work easier.
You just made my job harder. I said I would do it, and I already had it
done. Now I have to fix my code to interact with your changes. And,
due to the following, almost undo your code.
You didn't read your diff, you left one instance of accessing the static
variable, instead of calling the static accessor method; granted, it's
inside a comment, but still, you should have double-checked your diff.
I do.
This next one is my opinion: Putting 'ofbiz' into the method name is
superfluous. We already know it is an ofbiz class. No need to duplicate
that into the name of the method.
The method returns a static BeansWrapper. The name doesn't reflect that
fact. My local code has it named 'singletonBeansWrapper()'.
.
In summary, I'm rather annoyed. Normally, I wouldn't be. But in this
case, I said I would do this change, and you went and did it anyways. I
even gave a good reason for wanting to do it, as I needed it anyways to
implement the findByAnd fix.
