Repository: jprichardson/node-fs-extra
Pull request #517 (2017-11-09 21:35:08)

Create dest parent after applying filter in copy
When applying filter, I think this is a little bit better than before although it still highly depends on the user's filter conditions!

But at least, if users choose their filter conditions intelligently, this way, `copy()` correctly doesn't create dest parent directory since its child path fails on filter.

This is until @RyanZim adds his changes for filter function. 

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14127588/badge)](https://coveralls.io/builds/14127588)

Coverage remained the same at 86.1% when pulling **9639c695826d5d89fe288d6dd4eeafdc078a2613 on copy-parent-dir** into **b72ba64dbb94afb0416a25dfa5fdb2c21ca58f3d on develop**.

manidlou: That's true. Let me see what I can do here!
coveralls: 
[![Coverage Status](https://coveralls.io/builds/14128704/badge)](https://coveralls.io/builds/14128704)

Coverage increased (+0.01%) to 86.114% when pulling **8ca10745b843b70f0ffc8ff5eea04fea3a84a264 on copy-parent-dir** into **b72ba64dbb94afb0416a25dfa5fdb2c21ca58f3d on develop**.

manidlou: > Looks good; please squash to one commit.

Sure thing.

Patch:
diff --git a/lib/copy/copy.js b/lib/copy/copy.js
index 76aaa23..f270c1d 100644
--- a/lib/copy/copy.js
+++ b/lib/copy/copy.js
@@ -35,6 +35,8 @@ function copy (src, dest, opts, cb) {
   // don't allow src and dest to be the same
   if (src === dest) return cb(new Error('Source and destination must not be the same.'))
 
+  if (opts.filter && !opts.filter(src, dest)) return cb()
+
   const destParent = path.dirname(dest)
   pathExists(destParent, (err, dirExists) => {
     if (err) return cb(err)


Test patch:
diff --git a/lib/copy/__tests__/copy-prevent-copying-identical.test.js b/lib/copy/__tests__/copy-prevent-copying-identical.test.js
index 6435710..615645b 100644
--- a/lib/copy/__tests__/copy-prevent-copying-identical.test.js
+++ b/lib/copy/__tests__/copy-prevent-copying-identical.test.js
@@ -8,21 +8,21 @@ const klawSync = require('klaw-sync')
 
 /* global beforeEach, afterEach, describe, it */
 
-describe('+ copySync() - prevent copying identical files and dirs', () => {
+describe('+ copy() - prevent copying identical files and dirs', () => {
   let TEST_DIR = ''
   let src = ''
   let dest = ''
 
   beforeEach(done => {
-    TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-sync-prevent-copying-identical')
+    TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-prevent-copying-identical')
     fs.emptyDir(TEST_DIR, done)
   })
 
   afterEach(done => fs.remove(TEST_DIR, done))
 
   it('should return an error if src and dest are the same', done => {
-    const fileSrc = path.join(TEST_DIR, 'TEST_fs-extra_copy_sync')
-    const fileDest = path.join(TEST_DIR, 'TEST_fs-extra_copy_sync')
+    const fileSrc = path.join(TEST_DIR, 'TEST_fs-extra_copy')
+    const fileDest = path.join(TEST_DIR, 'TEST_fs-extra_copy')
 
     fs.copy(fileSrc, fileDest, err => {
       assert.equal(err.message, 'Source and destination must not be the same.')
diff --git a/lib/copy/__tests__/copy-prevent-copying-into-itself.test.js b/lib/copy/__tests__/copy-prevent-copying-into-itself.test.js
index d759a1a..9fe0ef0 100644
--- a/lib/copy/__tests__/copy-prevent-copying-into-itself.test.js
+++ b/lib/copy/__tests__/copy-prevent-copying-into-itself.test.js
@@ -21,43 +21,11 @@ const dat1 = 'file1'
 const dat2 = 'file2'
 const dat3 = 'file3'
 
-function testSuccess (src, dest, done) {
-  const srclen = klawSync(src).length
-  assert(srclen > 2)
-  fs.copy(src, dest, err => {
-    assert.ifError(err)
-
-    const destlen = klawSync(dest).length
-
-    assert.strictEqual(destlen, srclen)
-
-    FILES.forEach(f => assert(fs.existsSync(path.join(dest, f)), 'file copied'))
-
-    const o0 = fs.readFileSync(path.join(dest, FILES[0]), 'utf8')
-    const o1 = fs.readFileSync(path.join(dest, FILES[1]), 'utf8')
-    const o2 = fs.readFileSync(path.join(dest, FILES[2]), 'utf8')
-    const o3 = fs.readFileSync(path.join(dest, FILES[3]), 'utf8')
-
-    assert.strictEqual(o0, dat0, 'file contents matched')
-    assert.strictEqual(o1, dat1, 'file contents matched')
-    assert.strictEqual(o2, dat2, 'file contents matched')
-    assert.strictEqual(o3, dat3, 'file contents matched')
-    done()
-  })
-}
-
-function testError (src, dest, done) {
-  fs.copy(src, dest, err => {
-    assert.strictEqual(err.message, `Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`)
-    done()
-  })
-}
-
 describe('+ copy() - prevent copying into itself', () => {
   let TEST_DIR, src
 
   beforeEach(done => {
-    TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-prevent-copying-into-itself-4')
+    TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-prevent-copying-into-itself')
     src = path.join(TEST_DIR, 'src')
     fs.mkdirpSync(src)
 
@@ -370,3 +338,35 @@ describe('+ copy() - prevent copying into itself', () => {
     })
   })
 })
+
+function testSuccess (src, dest, done) {
+  const srclen = klawSync(src).length
+  assert(srclen > 2)
+  fs.copy(src, dest, err => {
+    assert.ifError(err)
+
+    const destlen = klawSync(dest).length
+
+    assert.strictEqual(destlen, srclen)
+
+    FILES.forEach(f => assert(fs.existsSync(path.join(dest, f)), 'file copied'))
+
+    const o0 = fs.readFileSync(path.join(dest, FILES[0]), 'utf8')
+    const o1 = fs.readFileSync(path.join(dest, FILES[1]), 'utf8')
+    const o2 = fs.readFileSync(path.join(dest, FILES[2]), 'utf8')
+    const o3 = fs.readFileSync(path.join(dest, FILES[3]), 'utf8')
+
+    assert.strictEqual(o0, dat0, 'file contents matched')
+    assert.strictEqual(o1, dat1, 'file contents matched')
+    assert.strictEqual(o2, dat2, 'file contents matched')
+    assert.strictEqual(o3, dat3, 'file contents matched')
+    done()
+  })
+}
+
+function testError (src, dest, done) {
+  fs.copy(src, dest, err => {
+    assert.strictEqual(err.message, `Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`)
+    done()
+  })
+}
diff --git a/lib/copy/__tests__/copy.test.js b/lib/copy/__tests__/copy.test.js
index 4787bfd..d8e38f6 100644
--- a/lib/copy/__tests__/copy.test.js
+++ b/lib/copy/__tests__/copy.test.js
@@ -208,6 +208,21 @@ describe('fs-extra', () => {
     })
 
     describe('> when filter is used', () => {
+      it('should do nothing if filter fails', done => {
+        const srcDir = path.join(TEST_DIR, 'src')
+        const srcFile = path.join(srcDir, 'srcfile.css')
+        fse.outputFileSync(srcFile, 'src contents')
+        const destDir = path.join(TEST_DIR, 'dest')
+        const destFile = path.join(destDir, 'destfile.css')
+        const filter = s => path.extname(s) !== '.css' && !fs.statSync(s).isDirectory()
+
+        fse.copy(srcFile, destFile, filter, err => {
+          assert.ifError(err)
+          assert(!fs.existsSync(destDir))
+          done()
+        })
+      })
+
       it('should only copy files allowed by filter fn', done => {
         const srcFile1 = path.join(TEST_DIR, '1.css')
         fs.writeFileSync(srcFile1, '')
@@ -234,7 +249,7 @@ describe('fs-extra', () => {
         })
       })
 
-      it('should should apply filter recursively', done => {
+      it('should apply filter recursively', done => {
         const FILES = 2
         // Don't match anything that ends with a digit higher than 0:
         const filter = s => /(0|\D)$/i.test(s)
@@ -280,7 +295,7 @@ describe('fs-extra', () => {
         })
       })
 
-      it('should apply the filter to directory names', done => {
+      it('should apply filter to directory names', done => {
         const IGNORE = 'ignore'
         const filter = p => !~p.indexOf(IGNORE)