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

Ignores are always dot:true #227

Merged
merged 2 commits into from
Nov 11, 2015
Merged

Ignores are always dot:true #227

merged 2 commits into from
Nov 11, 2015

Conversation

isaacs
Copy link
Owner

@isaacs isaacs commented Oct 25, 2015

@isaacs isaacs mentioned this pull request Oct 25, 2015
@gaboesquivel
Copy link

👍

@sindresorhus
Copy link
Contributor

👍

@isaacs
Copy link
Owner Author

isaacs commented Oct 26, 2015

If anyone wants to weigh in on the substantial change here, take a look at these test cases: https://github.com/isaacs/node-glob/blob/dot-ignore/test/ignore.js#L34-L39

The relevant file structure looks like this:

$ tree a/{x,z}
a/x
└── .y/
    └── b
a/z
└── .y/
    └── b

2 directories, 2 files

The linked test cases go [pattern, ignore, results]. So, eg, a pattern of a/**/.y/b, with an ignore set to a/x/** will prevent a/x/.y/b from matching, even though the ** would not normally match the .y path portion.

@aredridel
Copy link

This sure seems like simpler behavior

@isaacs
Copy link
Owner Author

isaacs commented Nov 11, 2015

Alright, the requisite time for objection and testing has passed, and it seems to make everyone's use cases better. I'm gonna go ahead and land this. It's a SemVer-major bump, because of the change in behavior, but it's the right change, I believe.

@isaacs isaacs merged commit 2f8b6d7 into master Nov 11, 2015
@UltCombo
Copy link

This does fix a bunch of issues and is indeed less surprising.

Though, I don't understand why the other rules for the ignore patterns wouldn't match the main matching pattern's rules? (#166)

Perhaps it is better to keep this simple, but having different rules for the matching and ignore patterns will likely lead to unexpected/counter-intuitive behavior.

@isaacs
Copy link
Owner Author

isaacs commented Nov 11, 2015

@UltCombo I don't understand the question.

I think experience thus far has shown that having ignore patterns not match dot files is always more confusing than the alternative, even when the main pattern doesn't have dot: true.

@UltCombo
Copy link

@isaacs I agree that ignore patterns being always dot: true makes sense. I was talking about the other Minimatch options, such as nocase, noglobstar, matchBase. Shouldn't the Minimatch instances for the ignore patterns have these options set to the same value as the matching glob pattern?

@thirdcreed
Copy link

node -p 'require("glob").sync("/home/thirdcreed/Projects/glob-stream/test/fixtures/*swag",{ignore:["/home/thirdcreed/Projects/glob-stream/test/fixtures/**"],dot:true,cwd: "/home/thirdcreed/Projects/glob-stream/test",cwdbase:false,nonull:false})'

This works for me in milliseconds, but this code times out on only one of the tests, with identical parameters. An end event is never emitted.

Any insight you can give me into what I might be doing wrong would be much appreciated.

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.

None yet

6 participants