-
Notifications
You must be signed in to change notification settings - Fork 152
refactor: use utility for cross-platform path regex construction #701
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
refactor: use utility for cross-platform path regex construction #701
Conversation
james-elicx
commented
Jan 16, 2025
- Create a new utility for constructing regular expressions with cross-platform paths.
- Update usage to use the new utility.
commit: |
🦋 Changeset detectedLatest commit: 8d603ae The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -0,0 +1,10 @@ | |||
export function getCrossPlatformPathRegex( | |||
regex: string, | |||
opts: { escape: boolean } = { escape: true }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the escape options ? It's not used anywhere.
My take on this is that we should only escape path related stuff here (i.e. /
the starting ./
and file extensions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the escape options ?
That's something I asked James.
If you only want to escape a path then you would use the default value (true
)
But sometimes (i.e. ESBuild filter), you might want to have more control on the pattern and do not escape special chars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@james-elicx thanks for the change, LGTM.
Let's have more discussion with @conico974 about the exact behavior this should have
@conico974 replying here as the convo is resolved: I understand your point but sounds like "only escape path related stuff here (i.e. / the starting ./ and file extensions)" is not without surprises either. For now I feel like we have those solutions:
I need some think to think about it. Notes:
|
@vicb As you said there is probably no ideal solution for this ( unless with some typescript wizardry, but it's not worth the effort ). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks for adding a clear doc and tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks