sql-highlight icon indicating copy to clipboard operation
sql-highlight copied to clipboard

Bug in beta tokenizer

Open Qtax opened this issue 1 year ago • 6 comments
trafficstars

Just looking at the code I think there is a bug here:

https://github.com/scriptcoded/sql-highlight/blob/909c361424255f5af2a5e87b96dcd9e93e61123d/lib/index.js#L42C21-L42C21

This only matches litteral periods .. I assume that you meant to match any character /(?<unknown>.+?)/.

Without this change (now that it's using .matchAll()) anything in the input SQL string that does not match the regex will be removed from the results and not visible to the user. I assume that is undesired.

(Also of note that this approach will at most match a single character in every unknown group match.)

And a bit off topic, but I'm using this highlighter with a non-standard SQL flavor, which uses several additional keywords. These keywords will now be highlighted as identifiers. I hope the highlighting for those is not too disturbing. 😅

Qtax avatar Jan 08 '24 16:01 Qtax

This issue has been marked as stale because it has been open for 14 days with no activity. Remove the stale label or comment or this will be closed in 5 days.

scriptsbot avatar Jan 23 '24 02:01 scriptsbot

This issue was closed because it has been stalled for 5 days with no activity.

scriptsbot avatar Jan 29 '24 02:01 scriptsbot

As good a time as any to give some form of response at least. Simply put I haven't had that much time the last few weeks due to various reasons. But next week is vacation, maybe I'll have some time then :smile: No promises though

scriptcoded avatar Feb 01 '24 11:02 scriptcoded

And a bit off topic, but I'm using this highlighter with a non-standard SQL flavor, which uses several additional keywords. These keywords will now be highlighted as identifiers. I hope the highlighting for those is not too disturbing. 😅

Covered by #189

scriptcoded avatar Mar 10 '24 13:03 scriptcoded

Woops, that does seem like a mistake, not sure why I put the backslash there. Or for that matter, the + sign.

wkeese avatar Apr 06 '24 13:04 wkeese

I submitted #193, it simultaneously fixes this ticket (which is a regression from my #148) and fixes #150.

wkeese avatar Apr 13 '24 23:04 wkeese

#193 is merged to beta and will be released soon. Closing this a bit prematurely. Feel free to reopen if you don't think it's fixed.

scriptcoded avatar Jul 02 '24 20:07 scriptcoded

:tada: This issue has been resolved in version 6.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

scriptsbot avatar Jul 02 '24 21:07 scriptsbot