Re: [Flamerobin-devel] RFC: Quotable names

Milan,

Milan Babuskov wrote:

> I was looking at our "major feature" list, and starting to think how to 
> implement the "Quotable identifiers (not ignoring case)" painlessly.

I have thought about quoted identifiers when thinking about the syntax 
scanner/parser that we need, and I believe we won't get them painlessly. 
  More than that, I think we need to invest a little time to clean up FR 
internals, before we implement any new features.

> So, in order to work with those objects, we need to quote their
> names. My idea is to do this as transparently as possible, without
> any intervention needed bu the user.

Agreed.

> I propose we add a new function MetadataItem::getQuotableName or
> getScriptableName, or something like that (I'm open to suggestions)

We should not decide whether a name needs to be quoted, but instead keep 
the DBH object names, quoted or unquoted, as we got them - either from 
the database, or from the user.  We should only ever *add* quotes when 
necessary (for instance when a reserved word in upper case letters is 
read as the column name from the database), but never remove quotes.

> We would, of course change all the statement-generating code to use
> this instead of getName(). We would still use unquoted name to show
> it in the tree, property pages, etc.

Again, we should show all names as they *are*.

> It would be good that isReserved and containsLowerCase are publicly 
> available static functions (we could call them some other name, or even 
> clamp them together), so that we can all those even if we only got a 
> string somewhere (for example: adding a new field).
> 
> I would leave two options for the user, though:
> 1. quote everything (off by default)
> 2. ignore case (on by default)
> 
> By "ignore case" I mean that when user opens FieldPropertiesFrame, adds 
> a new field giving it a name with lowercase letters, FR would not quote 
> it. It could be called "ignore case of user supplied names" or something 
> like that.

More user options are a bad idea. IMNSHO we are already bloating FR with
a lot of user options.  Adding more options for quoted identifiers isn't
going to fly, instead the code should be smart enough to decide for
itself whether quotes are needed.  For your example I would prefer a 
checkbox in the FieldPropertiesDialog over a global option.


Regarding my first comment:

A few things should IMHO be fixed before we attempt to create new 
features.  I could list many of those, but here are two that are closely 
connected with the quoted identifiers feature:

To really support quoted identifiers transparently we need to be able to 
compare DBH object names, regardless of where they come from.  Right now 
FR uses "==" everywhere to compare names, which is wrong for unquoted 
identifiers, and I believe there are any number of ways to trick the 
statement parsing in FR, just by playing with char cases and quotes.

The first fundamental change needed is the replacement of current

   wxString nameM;

in MetadataItem with something like

   Identifier nameM;

a class which has methods to compare with another Identifier / wxString 
/ std::string for equality.  This Identifier class would contain the 
name in exactly the given char case, and it would maybe have a field 
isQuotedM (maybe something like a tree-state: quoted, unquoted, 
as-needed (?)).

The second important change is the introduction of a syntax 
scanner/parser; without it support for quoted identifiers in 
parseStatements() would mostly introduce write-only code (the sort that 
can be written, but never again be read).

I know that this sounds kind of negative, and that this kind of 
refactoring is both difficult and brings no user-visible progress.  But 
I doubt that FR will stay maintainable when features are piled on top of 
  a foundation that isn't sound.

For me it isn't really maintainable right now.
I have to say that several times during the last weeks I started to work 
on some part of FR source code or another, only to give up after a 
couple of hours.  Either because my changes had side-effects I did not 
understand, or because necessary changes started to touch a lot of 
files, or because I felt not safe changing the internals of 
ExecuteSqlFrame.cpp (1956 lines at the time of writing...).

Do you really feel good with adding more code (== features) to FR, 
without cleaning up the base first?

Thanks

-- 
Michael Hieke