Skip to content

Bug in beta tokenizer #178

Closed
Closed

Description

@Qtax

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

Activity

  1. scriptsbot commented on Jan 23, 2024

    @scriptsbot
    Collaborator

    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.

  2. scriptsbot commented on Jan 29, 2024

    @scriptsbot
    Collaborator

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

  3. added
    no-stalePrevent making as stale
    and removed on Feb 1, 2024
  4. scriptcoded commented on Feb 1, 2024

    @scriptcoded
    Owner

    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 😄 No promises though

  5. scriptcoded commented on Mar 10, 2024

    @scriptcoded
    Owner

    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

  6. wkeese commented on Apr 6, 2024

    @wkeese
    Contributor

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

  7. added a commit that references this issue on Apr 10, 2024
    4a997ab
  8. wkeese commented on Apr 13, 2024

    @wkeese
    Contributor

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

  9. added 3 commits that reference this issue on Apr 14, 2024
    820029a
    0634d9b
    47884ee
  10. added 3 commits that reference this issue on Jul 2, 2024
    0bfef30
    db31b60
    07b58be
  11. scriptcoded commented on Jul 2, 2024

    @scriptcoded
    Owner

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

  12. added a commit that references this issue on Jul 2, 2024
    70af287
  13. scriptsbot commented on Jul 2, 2024

    @scriptsbot
    Collaborator

    🎉 This issue has been resolved in version 6.0.0 🎉

    The release is available on:

    Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions