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

add escapePath utility function #36

Merged
merged 16 commits into from
Sep 29, 2024

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Sep 11, 2024

closes #29

I didn't copy any tests over yet as I thought it was worth deciding on a couple of other questions first before investing more time into it

I wasn't quite sure how to handle the license. This project is ISC licensed and fast-glob is MIT licensed. The two licenses are extremely similar, but I think it'd be simplest if they used the same license. Would you be okay with switch to the MIT license? If so, we should probably also confirm with the other contributors to this project, which luckily shouldn't be hard to do given that it's still a relatively new project

I just put everything into the index file for now since it's still relatively small, but would you prefer these to live in a separate file? I took a few liberties in making minor modifications to the fast-glob code to keep things simpler. E.g. I replaced their utils.string.isEmpty method with directly doing item !== '' so that there were fewer methods to copy over - and I also find it easier to read without the extra layer of indirection.

Copy link

pkg-pr-new bot commented Sep 11, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/tinyglobby@36
pnpm add https://pkg.pr.new/tinyglobby@36

commit: c9c2ee2

@SuperchupuDev
Copy link
Owner

the current status on utility functions from fast-glob (and why i haven't merged this yet) is that i think copying code over isn't ideal and hopefully we find a way to implement these functions in a different way (ideally more optimal?). haven't really looked much into it though. if you manage to reimplement it though, please let me know

@benmccann
Copy link
Contributor Author

Yeah, that's a good call out. I've shrunk down the code dramatically now and removed probably 90% of the original. I think this is about as simple as it can get now

@SuperchupuDev
Copy link
Owner

interesting, i can notice there's something that could be improved: using a negative lookahead for the leading slash of the pattern, and removing the group names (we can always add them back later). also, .replaceAll was added in node 15, you'll have to use .replace with the global flag

@benmccann
Copy link
Contributor Author

I made the .replace change

I'm super nervous about touching the regex as it's not my strong suit and they seem pretty fiddly. I think we can try to improve it a bit, but I'd like to add some tests first. Of course the easiest way to do that would just be to copy the tests from fast-glob, but I kind of want to figure out the licensing thing first. Do you think it would be okay to change the LICENSE to MIT? If so, I can ping the other contributors to see if they're okay with it

@benmccann
Copy link
Contributor Author

Oh, I hadn't caught up on the Discord discussion yet. Looks like we're okay to proceed on that

Screenshot from 2024-09-16 13-15-49

@benmccann
Copy link
Contributor Author

benmccann commented Sep 16, 2024

Okay, I removed the group names and copied over some tests

I'm still too nervous to mess with the regex anymore though. There's not a lot of tests and I've never used a negative lookahead before. I'm not too worried about performance because this is usually just called once or twice in a program's life, so doesn't need to be super optimized. If you still think we should tweak it then I might have to leave that last bit to you

@SuperchupuDev
Copy link
Owner

don't worry, i'll look into it

@SuperchupuDev
Copy link
Owner

using a negative lookbehind seems to work and be more efficient, as it won't match already escaped characters. will test some more though
image

@SuperchupuDev SuperchupuDev changed the title copy escapePath from fast-glob add escapePath utility function Sep 18, 2024
src/utils.ts Outdated Show resolved Hide resolved
@SuperchupuDev SuperchupuDev merged commit cace1a1 into SuperchupuDev:main Sep 29, 2024
6 checks passed
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.

escapePath support
2 participants