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: Handle all valid ST characters #58

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

llimllib
Copy link
Contributor

@llimllib llimllib commented Sep 4, 2024

There is no OSC spec, but it appears that most emulators accept 0x1B 0x5C as an ST character, with 0x07 and 0x9C as alternatives with a bit less support.

This PR updates the regular expression to match all three of 0x1b 0x5C, 0x07, and 0x9C as an ST character, and updates the test file accordingly.

Choices I made, any of which I'm happy to change if you prefer:

  • I did not pull out the ST character part of the regular expression into a variable, though I would prefer that, because it didn't seem to match the style of the code that's already there
  • I did make the ST character into an array and loop over it in the tests, because that seemed the clearest. I can change it if you prefer
  • I did not add any comments with links to relevant documentation, because it seemed out of line with the style of the code

Closes #56

@llimllib llimllib force-pushed the llimllib/fix-osc-st-characters branch from 90f687f to e96b184 Compare September 4, 2024 16:02
According to wiki, all of [0x1b5c, 0x07, 0x9C] are valid
ST (string terminator) signals, so support them all.
@llimllib llimllib force-pushed the llimllib/fix-osc-st-characters branch from e96b184 to 2ae99de Compare September 4, 2024 16:04
@llimllib
Copy link
Contributor Author

llimllib commented Sep 4, 2024

The tests have passed, but I think that tsd is failing on irrelevant things that I don't understand?

@sindresorhus
Copy link
Member

I did not pull out the ST character part of the regular expression into a variable, though I would prefer that

Yeah, do that.

@llimllib
Copy link
Contributor Author

llimllib commented Sep 9, 2024

done, and merged main too. Thanks for fixing that up

@sindresorhus sindresorhus merged commit 9cba40d into chalk:main Sep 9, 2024
2 checks passed
@sindresorhus sindresorhus changed the title fix: handle all valid ST characters Fix: Handle all valid ST characters Sep 9, 2024
jasnell pushed a commit to nodejs/node that referenced this pull request Sep 21, 2024
PR-URL: #54865
Refs: chalk/ansi-regex#58
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit to nodejs/node that referenced this pull request Sep 21, 2024
PR-URL: #54865
Refs: chalk/ansi-regex#58
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to nodejs/node that referenced this pull request Oct 4, 2024
PR-URL: #54865
Refs: chalk/ansi-regex#58
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to nodejs/node that referenced this pull request Oct 4, 2024
PR-URL: #54865
Refs: chalk/ansi-regex#58
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#54865
Refs: chalk/ansi-regex#58
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#54865
Refs: chalk/ansi-regex#58
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit to nodejs/node that referenced this pull request Nov 16, 2024
PR-URL: #54865
Refs: chalk/ansi-regex#58
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit to nodejs/node that referenced this pull request Nov 16, 2024
PR-URL: #54865
Refs: chalk/ansi-regex#58
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit to nodejs/node that referenced this pull request Nov 17, 2024
PR-URL: #54865
Refs: chalk/ansi-regex#58
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit to nodejs/node that referenced this pull request Nov 17, 2024
PR-URL: #54865
Refs: chalk/ansi-regex#58
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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

Successfully merging this pull request may close these issues.

Unable to match links ended with \u001B rather than \u0007.
3 participants