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

Allow matching substrings instead of patterns #1035

Open
pihedron opened this issue Apr 27, 2024 · 3 comments
Open

Allow matching substrings instead of patterns #1035

pihedron opened this issue Apr 27, 2024 · 3 comments
Labels
a-2d Relates to the 2d package b-enhancement New feature or request c-accepted The issue is ready to be worked on

Comments

@pihedron
Copy link

Description

Currently, the Code.findAllRanges method does not match an exact substring in a Code object. Instead, it unnecessarily creates a RegExp object. If the findAllCodeRanges function can take a RegExp object as an argument, why should it have to convert a string to RegExp?

Solution

I could not find any existing function that matched a substring in the docs, so I am suggesting a quick change to remove some code from the findAllCodeRanges function in packages/2d/src/lib/code/CodeRange.ts.

// remove this block of code
if (typeof pattern === 'string') {
  pattern = new RegExp(pattern, 'g');
}

This means we can use code().findFirstRange('n * (n + 1) / 2') instead of typing out so many backslashes like this: code().findFirstRange('n \\* \\(n \\+ 1\\) \\/ 2'). It is probably better practice to use a RegExp constructor instead of implicitly converting a string into a RegExp.

@pihedron pihedron added the b-enhancement New feature or request label Apr 27, 2024
@aarthificial
Copy link
Contributor

aarthificial commented Apr 28, 2024

The current implementation of findAllCodeRanges uses String.matchAll to perform the search. This method expects a RegExp and passing a string to it will implicitly convert it to a RegExp just like we do.

If regexp is not a RegExp object and does not have a Symbol.matchAll method, it is implicitly converted to a RegExp by using new RegExp(regexp, 'g').

Removing the code you've mentioned will have no effect on the current behavior (aside from making the type checker angry).

It is probably better practice to use a RegExp constructor instead of implicitly converting a string into a RegExp.

The reason we convert strings to RegExps is to easily reuse the current algorithm. The point was never to let people define actual RegExps as strings and nowhere in the documentation do we suggest people do that.

Supporting strings without the need for escape sequences is a good idea but it will require more work than removing the mentioned lines of code. The current algorithm will have to be adapted to work with strings.

@aarthificial aarthificial removed their assignment Apr 28, 2024
@aarthificial aarthificial added c-accepted The issue is ready to be worked on a-2d Relates to the 2d package labels Apr 28, 2024
@pihedron
Copy link
Author

pihedron commented May 2, 2024

Yes, you are right. I forgot that String.matchAll converted string to RegExp anyway. Should I make a PR with a new function for finding a code range?

@pihedron
Copy link
Author

pihedron commented May 2, 2024

I have to admit motion-canvas is amazing though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-2d Relates to the 2d package b-enhancement New feature or request c-accepted The issue is ready to be worked on
Projects
None yet
Development

No branches or pull requests

2 participants