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

[Feature] glob/crawl by symlink path, not real path #83

Closed
IanVS opened this issue Oct 18, 2022 · 7 comments · Fixed by #84
Closed

[Feature] glob/crawl by symlink path, not real path #83

IanVS opened this issue Oct 18, 2022 · 7 comments · Fixed by #84

Comments

@IanVS
Copy link
Contributor

IanVS commented Oct 18, 2022

As noted in #23 (comment), currently globs match against resolved symlink paths, not the symlink paths themselves. This is a bit counter-intuitive, and I think that the symlink paths should be matched and returned instead, perhaps with an option to return real paths instead.

@thecodrr
Copy link
Owner

WIP at #84

@thecodrr
Copy link
Owner

@IanVS can you test against master branch and see if that works for you?

@IanVS
Copy link
Contributor Author

IanVS commented Oct 18, 2022

Yep, I copied over the src from master and it's now working correctly! Thanks!

@thecodrr
Copy link
Owner

Will release v5.3.0 then

@thecodrr
Copy link
Owner

@IanVS done. I changed the API a bit to be backwards compatible with v5 so check the docs to see how you can use it.

@IanVS
Copy link
Contributor Author

IanVS commented Oct 18, 2022

Thanks. One small piece of unsolicited feedback, is that this might fall into the boolean trap. When reading .withSymlinks(false) it could reasonably be thought that symlinks are disabled. An API like .withSymlinks({ resolve: boolean }) gives a bit more clarity, IMHO. But, that's just my own two cents. Thanks again for getting this released so quickly!

@thecodrr
Copy link
Owner

One small piece of unsolicited feedback, is that this might fall into the boolean trap.

I agree. I'll be making some breaking changes in v6.0 so this API is definitely not final. There's a lot of confusion in the current exposed methods so I will do some much needed renaming as well.

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 a pull request may close this issue.

2 participants