-
Notifications
You must be signed in to change notification settings - Fork 4
Fix glob bug. #157
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
Fix glob bug. #157
Conversation
|
N.B. In order to directly test #134, we'd have to create a zipfile with directory entries, which I'm not sure how to do within Python. |
Yeah, I tried writing such a file with the zipfile module, but you can't create "files" with trailing slashes. |
| self._test_matches( | ||
| 'Foo/**', | ||
| [ | ||
| 'Packages/Foo/', |
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.
Should we match this at all?
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.
Yes, because a UNIX glob would and I don't think we need to deviate. It's a corner case that probably won't come up very often, but if it does then I think handling it like a UNIX glob is the least surprising thing to do.
Co-authored-by: FichteFoll <fichtefoll2@googlemail.com>
Ugh. You're probably right, but still ugh. I've opened a question on Stack Overflow looking for an alternative. |
|
Do you think this can be merged as-is, punting the direct zipfile test to a future PR? |
FichteFoll
left a comment
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.
Yeah, we can merge this.
For #134.
After much research and testing, I determined that the fundamental bug was that every pattern component except for
**should match at least one character — so/A*B/should matchAB, butA/*shouldn't matchA/. This behavior should match other glob engines and resolve the linked bug.This behavior varies slightly from
sublime.find_resources('*'), which will find all resources (likesublime.find_resources('').It's probably bad news for other reasons if
sublime.find_resources()returns directories, but at least now it shouldn't be bad news for us in particular.