Re: Possible new issue introduced with 20140316 release

Hello, 

Sorry for the delay, I have been busy with other things.

The reason to switch to intern, is that intern will always return a symbol where read-from-string could return any valid lisp type.  Read-from-string will also stop reading if it reaches a space while valid symbols may contain spaces.  There are actually TODO's in each of these functions talking about checking types returned from `read-sql-value`.  If the package needs to be set, it seems fairly standard to bind *package* which will control where things are interned and can be handled in the users code.    That said, your argument is pretty compelling that if we print symbols readably and correctly prefixed with package then read-from-string is potentially a more robust solution as it allows a single column to contain symbols from many packages while binding *package* seems like it would m
 ost easily affect an entire query / column.  When making this change, I was thinking about reading in a string and ensuring it was a symbol symbol not reading in a readably printed symbol a!
 s a
symbol.  

Supporting that the code behaves at least somewhat sanely, the inverse operation `(database-output-sql 'cl-user::foo clsql-sys:*default-database*)` currently does not output symbols with their package designator.  I think that an argument could be made that this is also a bug, but I think that database-output-sql and read-sql-value should at least agree on how they handle symbols (which I think they currently do).  Keywords are printed strangely from database-output-sql and should probably be revisited.

I think a better function (than read-from-string) should be written that simply splits on ":" or "::" and interns the symbol into the correct package (and handling the keyword case obviously).  To my mind read-from-string is very general and much harder to deal with edge cases, where intern will either return a symbol or error out.  Please feel free to submit a patch that corrects the behavior of reading/writing symbols to handle packaged prefixed symbols.  Otherwise I will attempt to make a patch next time I am in this code base.  Also tests should be written to exercise this particular feature if they do not already exist.

I think an easy work around until then would be to set the column type to "string" instead and then use an after method on your classes to read-from-string/intern those columns.  Not beautiful, but I think reasonable and time effective.   If you feel very strongly that this change was in error, or was too much of a breaking change, then I could be persuaded to revert that particular change until such a time that we have better behavior, though that wont hit quicklisp for at least a month and probably wouldn't make it up to git hub for at least a week as I am about to go on vacation.

Thanks for the feedback, I hope this change has not caused you too many issues.  Unfortunately clsql is quite large and its tests, while nice, are not completely comprehensive.  Any single user is unlikely to exercise the entirety of it.  Thus breakages are somewhat inevitable though unfortunate.

Cheers,
Russ Tyndall
Acceleration.net


-----Original message-----
From: Mackram Raydan [email protected]
Date: Tue, 08 Apr 2014 09:20:02 -0400
To: Ralf Mattes rm-OuoMP45H64u6ogTlOYt/[email protected]
Subject: Re: [CLSQL] Possible new issue introduced with 20140316 release

Hey Ralf,

Thanks for taking the time to respond. I am aware of the changes (I had 
hoped to show my awareness of that in my email though it probably did 
not come across) although I was not aware that Russ was responsible for 
them.

With that said, I am all for the refactoring (as a matter of fact I used 
the example of clsql's refactor as an example of good software practice 
for my team here), however this does not answer why the change from 
*read-from-string* to *intern* .
The problem as I see it is by choosing intern we inherently imply that 
the symbol should come from the default package that the function intern 
takes as a keyword. By choosing read-from-string you allow the user to 
set any package he/she wishes by virtue that he can choose to pass the 
package:symbol-name to the db. That is what I was trying to refer to in 
my example, I hope I made it clear this time around (if not I will retry 
with a clearer code example)

Thank you

On 04/08/2014 04:55 PM, Ralf Mattes wrote:
> On Tue, Apr 08, 2014 at 04:08:03PM +0300, Mackram Raydan wrote:
>> ....
>> Can someone please help by shedding some light on why the change was done?
> Use your tools  :-)
>
> $ git blame oodml.lisp
>
>   ...
>   4f756ab5 sql/oodml.lisp    (Russ Tyndall       2013-06-20 15:12:31 -0400  623)        (when (< 0 (length val))
>   4f756ab5 sql/oodml.lisp    (Russ Tyndall       2013-06-20 15:12:31 -0400  624)          (intern (symbol-name-default-case val))))
>   4f756ab5 sql/oodml.lisp    (Russ Tyndall       2013-06-20 15:12:31 -0400  625)       ((smallint mediumint bigint integer universal-time)
>   ...
>
> So Russ did it in commit 4f756ab5.
> $ git log 4f756ab5
>
>   commit 4f756ab532ff033a34597a1c8030379e252952ca
>   Author: Russ Tyndall <[email protected]>
>   Date:   Thu Jun 20 15:12:31 2013 -0400
>
>      refactored read-sql-value to centralize this logic and reduce
>      overloading cases
>
>       * read-eval is off for all read-sql-value cases now
>       * the type=symbol case uses intern instead read-from-string
>
> Does this help?
>
>
> alf Mattes