Re: sql__quasiParser [patch]

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.


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.

2. I am not sure how I feel about simply evaluating a quasiliteral  
having side effects; on the one hand, it means they're not quasi- 
*literals*. On the other hand, if we think of `` as a more general  
construct for "embed another language into E", then imperative quasis  
are perfectly fine. And requiring
    sql`UPDATE ...`()
would be a bit noisy. (There is also precedent in the  
swtGrid__quasiParser, though not with good internal syntax.) After  
some minutes, I think I like the idea, especially seeing how clean the  
updoc session is, but I'd like some others' input on this.

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

    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.

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).


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).

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

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.


Code details:

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

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.

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

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

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

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?

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?


-- 
Kevin Reid                                  <http://switchb.org/kpreid/>