Re: sql__quasiParser [patch]

On Wed, 2010-03-10 at 13:41 -0500, Kevin Reid wrote:
> On Mar 10, 2010, at 10:53, Thomas Leonard wrote:
> > On 5 March 2010 13:36, Thomas Leonard <tal@it- 
> > innovation.soton.ac.uk> wrote:
> >> Hi all,
> >>
> >> I've written an sql__quasiParser, to provide easy and safe access  
> >> to SQL
> >> databases. It's on my "proposed" branch:
> >>
> >>  git pull git://gitorious.org/~tal-itinnov/repo-roscidus/it- 
> >> innovation.git proposed
> >
> > Any objections to me committing this to svn?
> >
> > http://gitorious.org/repo-roscidus/it-innovation/commit/753f8208bbc3aa38820a78163c235d10cee31753
> 
> 
> First of all, I'd just like to say that I'm glad you wrote this  
> library; it is in general concept very much what I have thought ought  
> to exist. The following are my comments from a brief scan of the  
> commit above.

Some updates based on your suggestions are here:

http://gitorious.org/~tal-itinnov/repo-roscidus/it-innovation/commits/sql

(as separate commits, but I'll squash them into one before committing to
svn, I think)

> Design issues:
> 
> 1. A question on my mind is whether it really ought to be part of E-on- 
> Java as opposed to an external library; however, seeing as it is  
> tightly tied to the Java SQL libraries, making it part of E-on-Java  
> seems reasonable. If you have the spare time, a specification and/or  
> test suite for "E SQL library" independent of Java would be a very  
> nice thing to have.

Apart from the initial setup step, I think the current updoc should work
as a generic test suite (perhaps with some changes to the exact error
messages).

By the way, I notice that many of the updoc tests in E are highly
system-specific. e.g.

$ updoc makeBootServer.updoc 

updoc: started
[...]
expr: def vatFile := makeAtomicFile(<file:~/Desktop>, "foo.vat")
original and new value differ
original value: <atomic: <file:c:/Documents and Settings/millerm1/Desktop/foo.vat>>
     new value: <atomic: <file:/home/tal/Desktop/foo.vat>>

Is Mark the only one who runs the tests?

[ 2. quasiparsers with side-effects ]

> 3. I think makeSql__quasiParser is too ugly a name for something that  
> people will use regularly; furthermore, the name "sql__quasiParser",  
> while people may use it that way, should not be enshrined since the  
> quasiparser *is* specific to a database connection.
>
>  I would rather see  
> either
>
>     a) "makeSQLQuasiParser" (the non-identity of the name to what you  
> would use it by suggests to the user to choose their name), or

Done. However, I'd like to encourage people to use the name sql``
because it makes syntax highlighting work easily. Otherwise, your text
editor has to understand the code to work out what syntax rules to apply
to which quasi strings. See the screenshot here for an example:

http://labs.it-innovation.soton.ac.uk/index.php?option=com_content&view=article&id=74:safe-database-access-in-e&catid=36:experiments

>     b) one object which wraps the connection which you then get the  
> parser from (this provides for other objects, also derived from the  
> Java connection, to be gotten without having passed the connection  
> around; this might not be useful for this application. In my XML  
> library design, this scheme provides for having a single namespace- 
> context which both XML and XPath quasiparsers may be had in.

While it might be useful, I think it would make sense for someone to add
that later as a separate object. e.g.

 def sqlTools {
   to getQuasiParser() {
     return <import:...makeSQLQuasiParser>(myConnection)
   }
   ...
 }

> 4. The name "singleton" feels too long and weighty: too much of a  
> distraction from the purpose of the code using it. How about "sole" or  
> "only"? Or, given the E convention for a zero-dimensional collection,  
> get/0 (and fetch/1 for the failure-thunk case).

I believe "singleton" is the correct term for a (result) set with
exactly one element:

http://en.wikipedia.org/wiki/Singleton_%28mathematics%29

get/0 doesn't work for me, because the singleton method rejects output
with multiple results, whereas I'd expect get to return everything. The
closest equivalent with lists is:

  def [item] := list

But this doesn't work too well with SQL result sets because we normally
lay out columns vertically, and also because rows are themselves lists,
so you'd get:

  def [[id]]] := sql`SELECT ...`

which seems uglier, and will give a less helpful error message when
there's no match.

> Formatting issues:
> 
> 1. The copyright header is missing the "...." which are a convenience  
> to mark the 79-column wrap limit.

> 2. Code in the E source tree should be wrapped to use at most 79  
> columns, and use four-space indents (not tabs).

Added. Though, is there any plan to re-evaluate this rule, given that
modern monitors tend to be wide-screen? 80-column wrapped text is very
bad on these, because it wastes the plentiful horizontal space but takes
up more of the precious vertical space :-( Likewise for the trace log
messages.

> 3. On line 61: As a matter of style, do not use spaces inside an ==  
> pattern: emphasize that it is a unary prefix operator.

Done.

> 4. I mildly object to your src/licenses/NOTICE.txt. Generally, we have  
> source with many different origins and licenses in the project; the  
> expectation is that each file gives its own copyright and license  
> information. MIT is merely the preferred license for new code. As  
> such, the only additional thing that might be added to src/licenses is  
> a statement to this policy.

OK, I've added some text to the start of the file to explain this.

Our policy (which is an alternative to including the previous gigantic
header you complained about last time ;-) assumes that the project
already has a file listing the current copyright holders, and just gives
some boilerplate text to put on the end.

> Code details:
> 
> 1. Why not import java.sql.Types instead of copying in the constant  
> values?

Types is currently considered unsafe. I suspect that allowing read-only
access to final static ints is probably safe, though. Maybe this could
be done automatically?

> 2. valueMaker/1 for quasiparsers is deprecated; use the valueMaker/2  
> interface which uses single characters only for hole markers, thus  
> simplifying the parsing job. Use org.quasiliteral.text.convertHoles to  
> adapt to the old interface.

Done.

> 3. Have you reviewed the feasibility of using Twine source-span  
> information in any parse errors that may occur?

Any parse errors will be coming from Java. The stacktrace seems to give
enough information.

However, I have noticed that files which refer to undefined variables in
their quasi-literals tend to give the importing file as the location of
the error, which is annoying. Mismatched brackets also seem hard to
locate from E error messages.

> 4. It would be good to add appropriate 'implements' clauses for the  
> quasiparser and its products for the interfaces they implement.

Done (does this have any actual effect?).

> 5. On line 56: You've already coerced to a String s, so special  
> measures to avoid Twine should be unnecessary: explain?

The comment was trying to explain the reason why we needed to check for
things-that-coerce-to-strings in the first place. I've clarified it.

> 6. Does the Java interface guarantee to fail promptly in the event  
> that a result set is already closed; that is, if sqlResults is  
> iterated over a second time?

I don't know, but it seems likely. HSQLDB behaves that way in my tests.
If a database somehow implemented close as a noop, you still wouldn't
get any results because you'd already be at the end of the result set.

> 7. In sqlValue.substitute, how about a variable bound to i + 1 so that  
> it's clearer why the offset is used and where it is?

Done.


-- 
Dr Thomas Leonard
IT Innovation Centre
2 Venture Road
Southampton
Hampshire SO16 7NP

Tel: +44 0 23 8076 0834
Fax: +44 0 23 8076 0833
mailto:tal-v5nx5w6akNyLE8xUarVfuPLx9OUvmyODWmv/[email protected]
http://www.it-innovation.soton.ac.uk