Marc DEXET schrieb:

Jochen Theodorou wrote:
Hi,

No new issue needed I would say. Also the code looks good, there are
just a few small things...

I have updated the code while taking into account your remarks
 

(1) there is no documentation on the code

You would say documentation on new methods or long, textual and contextual documentation ?
I have put javadoc documentation.

I mean the missing javadoc for the methods you added to Sql... I see you have attached a complete Sql class today... please show me only the patch. the sql class is far to big to see where exactly you made changes.

conceptual documentation is also always welcome. Maybe you want to write some examples in our wiki... there is http://groovy.codehaus.org/Database+features and http://docs.codehaus.org/display/GROOVY/Tutorial+6+-+Groovy+SQL which you may want to check. Of course this is no requirement for us to accept the patch.

(2) maybe connectionPrepareStatement, createCache, getCachedStatement
should be private?

Done

(3) Aaron Digulla mentioned, that we need to
ensure that no wrong connection will be used... does the code ensure
this?

You're thinking about dataSource's connection?

ehm no ;) I am by no means a sql expert, I am used to OODBMs and raw SQL, but not all the jdbc stuff ;) So I don't exactly know what Aaron means, I was just asking.

If we're doing statement caching, current connection must not be closed and keep as long as you have to. So we have just to consider dataSource's connection as passed as parameter connection (see property groovy.sql.Sql.useConnection)
When caching is over, connection is normally closed.

which means we can not make caching into a general feature for Sql

(4) the usage of the word persistent is probably a bit
misleading... at last without any documentation
(6) if holdOn is there, do we need a public API for persistent?
(7) we probably need to find a better name for holdOn ;) How about
cacheStaments for example?

Ok ok :)

hehe, this was just a suggestion, I am usually bad with names ;)

holdOn is replaced par cacheStatement(Closure closure) and setPersistent(Boolean b) is repalced by cacheStatement(Boolean b).

>

cacheStatement(Closure closure) is usefull for request nesting in the closure-look way
Sql.cacheStatement {
       sql.eachRow(...) {
               sql.eachRow(...)        }
}

which makes me think that we should maybe turn this into an builder, so you won't have to write "sql." all the time.

cacheStatement(Boolean b) is usefull for just avoid wasting resource
sql.cacheStatement = true
sql.eachRow(...) {}
...
sql.eachRow(...) {}
...
sql.cacheStatement = false

somehow I don't like this very much... most of the time we try to limit the scope of resources automatically. your cacheStatement(Closure) does this very good. But here... What happens if the last line is forgotten? I have to think about that part a bit... I will take a close look at it once you have attached the patch

[...]

(8) maybe this should be a general feature
and we should use a property
that enables caching for all code in Sql.java

In fact this feature now allows to cache statements, preparedStatement and not to waste connection.

ok

I get now Sql patch and unit tests. I put them in the same issue ?

yes, please... maybe we can delete the older files

bye blackdrag

--
Jochen "blackdrag" Theodorou
The Groovy Project Tech Lead (http://groovy.codehaus.org)
http://blackdragsview.blogspot.com/
http://www.g2one.com/

---------------------------------------------------------------------

To unsubscribe from this list, please visit:

  http://xircles.codehaus.org/manage_email