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

Fix negative prefixed positive tests #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bcomnes
Copy link

@bcomnes bcomnes commented Feb 6, 2022

Here is a problem I noticed:

!(node_modules)**/*.{png,ico} doesn't match anything in node-glob-ignore, but it does match when passed directly to glob. (E.g. any png or ico file in a subdirectory in the current directly that's not inside of node_modules, and no deeper).

This is due to this line dropping the glob:

// or only provide a negative pattern
if (positivesCount === 0) {
return {
result: []
}
}

Now, I could pass in ./!(node_modules)**/*.{png,ico} or **/!(node_modules)**/*.{png,ico} (probably the more desirable glob) but that starts to diverge from globs that work in glob.

I know its better to use the ignore array here, but I'm trying to generally retain the same API of a tool that was formerly using glob.

I'm thinking that's a bug in the design of that function, but I'm curious what you think. Perhaps checking to make sure the negative glob isn't for a group, which would indicate that it is in fact a positively matching glob. That is the fix I added in this PR but looking for your thoughts on the solution.

Here is a problem I noticed:

`!(node_modules)**/*.{png,ico}` doesn't match anything in `node-glob-ignore`, but it does match when passed directly to `glob`.  (E.g. any png or ico file in the current directly that's not inside of node_modules)

This is due to this line dropping the glob:

https://github.com/kaelzhang/node-glob-gitignore/blob/c8a236a52a14e6d1e8ba692c4a210dc79e817e7e/src/util.js#L83-L88

Now, I could pass in `./!(node_modules)**/*.{png,ico}` or `**/!(node_modules)**/*.{png,ico}` (probably the more desirable glob) but that starts to diverge from globs that work in `glob`.

I know its better to use the ignore array here, but I'm trying to generally retain the same API of a tool that was formerly using glob.

I'm thinking that's a bug in the design of that function, but I'm curious what you think. Perhaps checking to make sure the negative glob isn't for a group, which would indicate that it is in fact a positively matching glob. That is the fix I added in this PR but looking for your thoughts on the solution.
@bcomnes
Copy link
Author

bcomnes commented Feb 11, 2022

Hey, just checking back in to see if you have any thoughts on this approach.

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.

1 participant