Re: sql__quasiParser [patch]

On Mar 11, 2010, at 6:59, Thomas Leonard wrote:

> By the way, I notice that many of the updoc tests in E are highly
> system-specific. e.g. [...] Is Mark the only one who runs the tests?

It's more a case of that they've never been reviewed for actually  
being a proper test suite. Such things as this should be fixed (and  
can be by interp.setPrintFunc to hide the variations, for example; see  
nested-throwable.updoc)

> Done. However, I'd like to encourage people to use the name sql``
> because it makes syntax highlighting work easily.

Good point.

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

But what the method returns is not the set, but its element, so the  
name is wrong by that system.

> 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 expectation of get/N is that it returns an element of a  
collection. get/0 returns the sole element of a collection; for  
example, the value of a slot.

> 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 :-(

Please take up formatting issues with MarkM; he is the primary  
advocate for the current standard.

(I, for one, prefer to write code designed for soft-wrapping with no  
rigid length limit but *most* stuff around 80 cols -- I work with many  
windows open, and long-but-ugly-when-wrapped code means that  
somebody's hogging my screen.)

> Likewise for the trace log messages.

This is a separate issue.

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

I still don't like this incomplete-list-of-copyright-holders notion --  
I'd rather see it strictly decentralized. I think I'd better get  
MarkM's opinion on this.

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

The proper fix for this is to write the safej file. Use scripts/ 
safejTemplate.e as a starting point.

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

I don't know enough to judge this idea. However, I note that static  
fields are exposed on the maker object, and taming often considers the  
maker to be a privileged object, so this is difficult to do in general.

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

In practice, it's documentation for the reader. In theory, it should  
become the case that it is possible to find out whether a given object  
claims to implement that interface, e.g. for IDE/REPL purposes.

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

This is bad because it is a silent failure / variation of the contract  
depending on implementation, and iterate/1 only working once is  
unusual behavior. Please add an explicit throw and a test in the updoc  
for it.

To fit in with the structure of the problem and avoid complicating the  
code, I suggest moving 'var i := 0' next to 'def rs ...' and then  
adding a test in iterate/1 for i being nonzero.

Separately, I think it would be good to add asList() to the result  
interface for applications which want to examine the list multiple  
times. For extra credit, implement it in terms of  
makeConstList.fromValuesOf after porting that method from E-on-CL.

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