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

feat(regexp): add escape function #3334

Merged
merged 3 commits into from
May 11, 2023
Merged

Conversation

lionel-rowe
Copy link
Contributor

Fixes #3330

Escaping is fairly conservative to account for context sensitivity and forward compatibility with the v flag — the only non-alphanumeric printable ASCIIS that aren't escaped are "'_, and of the characters that are escaped, almost half use the most conservative \x hex notation. That could be reduced a little (but not much) by ignoring characters in ClassSetReservedDoublePunctuator, which only need escaping if repeated adjacent to each other in a within a character class in a v-flagged regex.

@kt3k
Copy link
Member

kt3k commented Apr 30, 2023

Thanks for the suggestion!

I think the directory name should be regexp/ instead of regex/ as JS has RegExp class, and also the API name should be regexpEscape (or regExpEscape?) instead of regexEscape. What do you think?

@lionel-rowe
Copy link
Contributor Author

I think the directory name should be regexp/ instead of regex/ as JS has RegExp class, and also the API name should be regexpEscape (or regExpEscape?) instead of regexEscape. What do you think?

This is probably one of those "gif/jif" things that everyone has their own strong opinions about, but it seems like regex is the more common usage within the community at large by a significant margin (a Google search for "javascript regex" gives 597k results, whereas "javascript regexp" gives 175k; meanwhile on GitHub, regex gives 6,970 JS repository results, whereas regexp gives 2,063).

Still, this function would typically be used when interpolating/concatenating content into strings passed to the RegExp constructor, so that's something in favor of regexp. Also, grepping for regex gives results for both variants, whereas grepping for regexp only gives results for regexp.

I started writing this answer arguing in favor of regex, but I think I've convinced myself that regexp is the better option 😅 happy to change unless anyone has a more convincing argument for regex.

@iuioiua
Copy link
Collaborator

iuioiua commented May 2, 2023

Still, this function would typically be used when interpolating/concatenating content into strings passed to the RegExp constructor, so that's something in favor of regexp. Also, grepping for regex gives results for both variants, whereas grepping for regexp only gives results for regexp.

+1 for regexp

@kt3k
Copy link
Member

kt3k commented May 4, 2023

@lionel-rowe Thanks for updating to std/regexp!

I now wonder if we could make API name just escape instead regExpEscape. I think that's more aligned to the rest of deno_std. (For example, parse in std/yaml not yamlParse, format in fmt/bytes instead of formatBytes (or bytesFormat), etc

@lionel-rowe
Copy link
Contributor Author

@lionel-rowe Thanks for updating to std/regexp!

I now wonder if we could make API name just escape instead regExpEscape. I think that's more aligned to the rest of deno_std. (For example, parse in std/yaml not yamlParse, format in fmt/bytes instead of formatBytes (or bytesFormat), etc

Done and done!

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@kt3k kt3k changed the title feat(regex): Add regexEscape function feat(regexp): add escape function May 5, 2023
@kt3k kt3k merged commit f6f8d20 into denoland:main May 11, 2023
7 checks passed
mxdvl pushed a commit to mxdvl/deno_std that referenced this pull request May 16, 2023
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.

Regex escaping
3 participants