Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
redtopia opened this issue Jul 20, 2018 · 13 comments
Open

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

redtopia opened this issue Jul 20, 2018 · 13 comments

Comments

@redtopia
Copy link

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Author

redtopia commented Aug 2, 2018

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
Copy link
Owner

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

@redtopia
Copy link
Author

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

@jcberquist
Copy link
Owner

@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
Copy link
Author

redtopia commented Sep 4, 2018

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
Copy link
Owner

@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
Copy link
Owner

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

@redtopia
Copy link
Author

redtopia commented Sep 4, 2018

That looks a lot better! 👍

@KamasamaK
Copy link
Contributor

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
Copy link
Author

redtopia commented Sep 4, 2018

@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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants