Repository navigation
Conversation
FotoVerite
left a comment
FotoVerite
left a comment
There was a problem hiding this comment.
The changes to the index look fine too me, I want to know what you're plans for the commented out tests? Future PR, remove, another commit.
| assert.deepStrictEqual(gp("(a|b)"), "."); | ||
| assert.deepStrictEqual(gp("/(a|b)"), "/"); | ||
| assert.deepStrictEqual(gp("./(a|b)"), "."); | ||
| // assert.deepStrictEqual(gp("(a|b)"), "."); |
There was a problem hiding this comment.
What is the actual plan for Universal Naming Conventions?
There was a problem hiding this comment.
I am not sure what you mean by "Universal Naming Conventions", but this isn't a valid glob. I believe it is representing an "extglob" but those require a preceding symbol. Our old implementation didn't validate patterns were actually globs, just glob-like. By changing to the new implementation that forces a full minimatch parse, we can determine it is not a valid glob.
I believe a true pattern for this glob will be {a,b} or perhaps a different glob.
Minimatch has been chosen by Node.js as the implementation of globbing they include in the runtime. This means that our glob comparisons need to match whatever minimatch decides. As of the time of this PR, the Minimatch included in Node 22 is 10.2.4 so that is the pinned version here.
This means there will be some breaking changes in the "glob parents" that are determined, but I went through each breaking test and believe each one to be correct. I will also be migrating commented-out tests to explicit tests showing the difference or annotating them with comments.
Note: Minimatch is doing a full glob parse as part of each call to glob-parent which may or may not be slower than our previous implementation. Due to the amount of issues opened on this repo and the gulp repo's above it related to incorrect glob parents, we are choosing to optimize for correctness instead of performance.
Fixes #60