feat: SQL comments support and new tokenizer

[Qtax]

Add SQL comments support.

Faster and simpler tokenization.

Refs: #133

[scriptcoded]

Thanks, looks like interesting changes! Haven't had time to read it through properly yet, but I've updated the commit lint rules on master so when you get the time, please feel free to rebase from master and that check should pass.

Quick look at the comment regex tells me it would be trivial to add the # comment format used by MySQL I mentioned in #133 (comment), right? https://dev.mysql.com/doc/refman/8.0/en/comments.html

# Comment
-- Comment
/* Comment */

[Qtax]

Added # comments support and rebased.

Also improved the escapeHtml function.
Edit: https://jsben.ch/oFiBQ https://jsben.ch/rqaQ0 (regex d0a760e is faster when there are "fewer" replacements)

[parallels999]

\x1b[2m doesn't work

[Qtax]

@parallels999 in what way does it not work? Work fine for me, both dark and light theme.

[erikn69]

Why did you change it?? \x1b[90m was great, now, there's no difference


Maybe better with italic \x1b[90m\x1b[3m, or \x1b[2m\x1b[90m\x1b[3m

Depends on where you use it, \x1b[2m is not supported in some scenarios

[PaolaRuby]

\x1b[2m doesn't work

+1 8824a8

[Qtax]

Why did you change it?? \x1b[90m was great, now, there's no difference

Changed it because 90 doesn't work on default light theme in VS code:

So apparently 90 doesn't work in some cases, and 2 doesn't in some others.

\x1b[2m\x1b[90m does work in VS code. Does that work for you guys?

[parallels999]

\x1b[2m\x1b[90m does work in VS code. Does that work for you guys?

it works for me 👍, but with italic looks awesome \x1b[2m\x1b[90m\x1b[3m

Also work in browsers, cmd console, powershell

[Qtax]

it works for me 👍

Great!

italic looks awesome \x1b[2m\x1b[90m\x1b[3m

Italic is harder to read in smaller fonts, so I personally would like to avoid it.

[scriptcoded]

I've read through the code now, and wow, good job! You really managed to squeeze together that getSegments quite a bit! I also really like the use of named groups instead of my overly complicated solution 👍

Split the commits

Could you please split the comment support and the performance improvements into two separate commits when we're done? I'll rebase this PR onto master in the end and since they're separate features I'd like them clearly separated in the commit history and the changelog.

Escaper performance

I tried it out with some more realistic real world data. The switch solution seems to be the most performant one. The difference in performance between escSwitch and escSwitch2 can probably be considered within margin of error, so I say let's go for the escSwitch implementation with inline replacer function.

My benchmark: https://jsben.ch/eGHEk

Comment color

\x1b[2m\x1b[90m, albeit being a bit chaotic, seems to work in multiple scenarios for me too. I say we go with this. As for italic I'd rather keep it simple and bare bones, and if someone wants italic they can use the options for that. Thanks for the input everybody!

[parallels999]

This works too, no highlighters const, no getRegexString function, no regex spaces

tokenizer = new RegExp('(.*?)(' + [
    '\\b(?<keyword>' + keywords.join('|') + ')\\b',

    /\b(?<number>\d+(?:\.\d+)?)\b/,

    // Note: Repeating string escapes like 'sql''server' will also work as they are just repeating strings
    /(?<string>'(?:[^'\\]|\\.)*'|"(?:[^"\\]|\\.)*"|`(?:[^`\\]|\\.)*`)/,

    /(?<comment>--[^\n\r]*|#[^\n\r]*|\/\*(?:[^*]|\*(?!\/))*\*\/)/,

    // Future improvement: Comments should be allowed between the function name and the opening parenthesis
    /\b(?<function>\w+)(?=\s*\()/,

    /(?<bracket>[()])/,

    /(?<special>!=|[=%*/\-+,;:<>])/,
 ].map(v => v.toString().replace(/^\/|\/\w*$|[\t ]+/g, '')).join('|') + '|$)', 'isy'),

[scriptcoded]

This works too, no highlighters const, no getRegexString function, no regex spaces

@parallels999 Yes, that would probably work. And even though it's more compact and could maybe be considered "cleaner", I think it adds to the obfuscation and makes the code harder to follow. I'd like to prioritize readability over compactness.

[Qtax]

Could you please split the comment support and the performance improvements into two separate commits when we're done? I'll rebase this PR onto master in the end and since they're separate features I'd like them clearly separated in the commit history and the changelog.

So you want it split into commits feat: SQL comments support and perf: improved tokenizer? Sure.

[Qtax]

I'd like to prioritize readability over compactness.

Agreed!

And JYI @parallels999 you want to remove |[\t ]+ in the replace there if you remove the spaces in the expression manually.

[parallels999]

@Qtax it's just an example, I do not have access to modify your PR

[scriptcoded]

Hey again! Just letting you know that I'll have to let this sit another few days as I've got quite a lot to do at the moment. Sorry for the holdup!

[Qtax]

@scriptcoded np, I've split the commits. Also wanted to add the improved escape function, but jsben.ch has been down all day for me. :-/

[wkeese]

Glad to see this change because it looks like you're fixing a bug with the old tokenizer: AFAICT, the old tokenizer would (incorrectly) highlight keywords/symbols inside of strings, for example

select * from EMP where DEPT="select * from EMP";

It was an architectural problem because the tokenizer ran the regexps one by one, whereas you need to tokenize with a single regexp.

I hadn't seen that named capturing groups feature before, took me a while to figure it out but it works nicely.

[scriptcoded]

@Qtax So I've read through everything one more time now and ran it past some colleagues as well. So as soon as #141 (comment) is fixed I think we're ready to merge! I would push a commit myself but since you've been force pushing I'd rather not mess things up for you.

[scriptcoded]

So you want it split into commits feat: SQL comments support and perf: improved tokenizer? Sure.

@Qtax and just to explain I don't necessarily need the feature split into two commits specifically, but at least commits that make sense. So that when this gets merged each commit is contained and releasable. In this specific case I think the easiest is therefore to just keep it as two commits.

[Qtax]

@Qtax So I've read through everything one more time now and ran it past some colleagues as well. So as soon as #141 (comment) is fixed I think we're ready to merge! I would push a commit myself but since you've been force pushing I'd rather not mess things up for you.

Fixed.

Force pushing since I don't want to make/keep unnecessary/unwanted commits, and prefer to update the commits instead. Especially if it's gonna be fast forwarded when merged. (Unless there is some other way to do that that I'm not aware of?)

[scriptcoded]

There we go, finally merged! Sorry for the holdup and huge thanks for the PR and all input! 🎉

Closes #133