jcberquist / sublimetext-cfml

CFML (ColdFusion and Lucee) package for Sublime Text
MIT License
115 stars 24 forks source link

Fix/enhance syntax highlighting for lower case SQL statements #119

Open redtopia opened 6 years ago

redtopia commented 6 years ago

Currently, syntax highlighting for lower case sql statements appears to work, but isn't triggered unless the first command is uppercase.

Here's an example: sql-syntax

jcberquist commented 6 years ago

The reason this is happening is that the syntax highlighting looks for SQL keywords at the beginning of any string, and if one is found, the string is highlighted as SQL. This allows for highlighting SQL in strings without the complications of trying to determine whether a string is expected to be SQL (e.g. inside of the queryExecute() call). Because of the way this works (on any string) I am a little unsure about using lowercase keywords to detect SQL. An uppercase SELECT starting a string makes it very likely that the string is SQL, but I think looking for lowercase keywords makes false positives much more likely.

redtopia commented 6 years ago

That makes sense. I wonder if anything could be done to improve the logic. I love the sql syntax highlighting, but all too often I'm working with lowercase strings.

redtopia commented 6 years ago

In addition to your current logic, could you add a case-insensitive regex that tests for common sql tokens like: (outer|inner) join, select \* is not null, is null, order by, etc.

Not only would it solve for the lower case issue, but you would also add highlighting to sql fragments when queries are being created in parts.

As far as false positives go, maybe you could make this configurable somehow. I personally wouldn't mind if sql syntax highlighting were added to a non-sql string that matched some sql tokens. But maybe some projects would be hindered by it, in which case maybe they could edit the regex, or some settings that generate the regex.

jcberquist commented 6 years ago

Hey @redtopia still thinking about this. I might give your suggestion above a try and see what happens.

redtopia commented 6 years ago

Cool... thanks! I'll give it a try. LMK.

jcberquist commented 6 years ago

@redtopia I have added some more lowercase sql highlighting. You can look at syntax_test_sql_strings.cfm to get an idea of what is supported now. Some of what is supported will only be available on multiline strings, but I did try to get some single line support in there as well.

redtopia commented 6 years ago

Thanks John. The changes aren't really doing much for my codebase... I have a lot of this going on, and from what I can see, none of our lower case queries are highlighted with your current changes:

    var q = queryExecute(sql="
        select a.companyType as companyTypeID, b.companytype
        from tblCompaniesType a
            inner join tblcompanytypes b on b.companytypeid =  a.companyType
        where a.companyID = :pk_param
        ", 
        params=[{ name="pk_param", cfsqltype="cf_sql_other", value=this.companyID }],
        options={
            datasource:variables.dsn
        }
    );
jcberquist commented 6 years ago

@redtopia: that is helpful, thanks. I am not going to be able to catch all cases - it is guessing after all. But I can add support to the "select" matching for column aliases.

jcberquist commented 6 years ago

I have added another commit to support basic column aliasing after the select.

redtopia commented 6 years ago

That looks a lot better! :+1:

KamasamaK commented 6 years ago

I agree that guessing whether a generic string literal is a SQL statement should be done conservatively. However, given this example, it is clear that any string literal value of the sql parameter should be a SQL statement.

redtopia commented 6 years ago

@KamasamaK, I agree, though I'm using named arguments here, and it's entirely possible to call queryExecute() without using named attributes, which makes the problem a bit more complicated.

KamasamaK commented 6 years ago

@redtopia Sure, but only a bit more complicated since when not using named arguments, the first argument would have the same rules applied. Both of these cases were actually addressed for queryexecute previously, but I'm not sure when it was removed.