Skip to content

Commit

Permalink
negated match goes into subdirectories
Browse files Browse the repository at this point in the history
 - fixes #16
  • Loading branch information
ferhatelmas committed Apr 15, 2015
1 parent 7968c80 commit e2f2ea0
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 12 deletions.
24 changes: 12 additions & 12 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,23 +23,14 @@ function readdir(path, ignores, callback) {

var ignoreOpts = {matchBase: true}
files.forEach(function (file) {
for (var i = 0; i < ignores.length; i++) {
if (minimatch(p.join(path, file), ignores[i], ignoreOpts)) {
pending -= 1
if (pending <= 0) {
callback(null, list)
}
return
}
}

fs.lstat(p.join(path, file), function (err, stats) {
if (err) {
return callback(err)
}

file = p.join(path, file)
if (stats.isDirectory()) {
files = readdir(p.join(path, file), ignores, function (err, res) {
files = readdir(file, ignores, function (err, res) {
if (err) {
return callback(err)
}
Expand All @@ -52,7 +43,16 @@ function readdir(path, ignores, callback) {
})
}
else {
list.push(p.join(path, file))
for (var i = 0; i < ignores.length; i++) {
if (minimatch(file, ignores[i], ignoreOpts)) {
pending -= 1
if (pending <= 0) {
callback(null, list)
}
return
}
}
list.push(file)
pending -= 1
if (!pending) {
callback(null, list)
Expand Down
16 changes: 16 additions & 0 deletions test/recursive-raddir-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,4 +72,20 @@ describe('readdir', function() {
done()
})
})

it('works when negated ignore list is given', function(done) {

var expectedFiles = [
__dirname + '/testdir/c.txt',
__dirname + '/testdir/d.txt',
__dirname + '/testdirBeta/ignore.txt'
]

readdir(__dirname, ['!*.txt'], function(err, list) {
assert.ifError(err);
assert.deepEqual(list.sort(), expectedFiles,
'Failed to find expected files.')
done()
})
})
})

2 comments on commit e2f2ea0

@Ajedi32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So correct me if I'm wrong, but doesn't this prevent ignores from matching directories at all? E.g. If I have a file structure a/b/c/<files> and I want to ignore everything in c, Ignoring "a/b/c" won't work? I'm not sure I like this change. It's definitely a breaking change anyway...

Though I guess ignoring a/b/c/** would still work, and this does make it possible to distinguish between files and directories, so maybe it's not a bad change after all. Idk...

@Ajedi32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing to consider is that this change does make having an ignore option in this library more a convenience than a necessity.

Prior to this change there were good performance reasons for having ignore functionality inside the library, since ignoring a directory would prevent recursive-readdir from recursing into that directory. With this change, recursive-readdir will recurse into the directory and call lstat on all the files inside it regardless of whether those files are ignored or not. That sort of functionality could easily be implemented outside this library with a combination of Array.prototype.filter and minimatch. It doesn't necessarily need to be in the library itself.

Maybe an option should be exposed to determine what behavior to use? If we do that, and make ignore match directories by default, then the next version of this library wouldn't even need to be a major release, since it wouldn't have this breaking change.

Please sign in to comment.