From d905bcdd6283c464078017d771416e0b05e1aca0 Mon Sep 17 00:00:00 2001
From: Blaine Bublitz <blaine.bublitz@gmail.com>
Date: Wed, 6 Jul 2016 14:42:18 -0700
Subject: [PATCH] Update: Make internals consistent

---
 lib/default-value.js                          |  9 ++--
 lib/dest/index.js                             | 14 ++++---
 lib/dest/write-contents/index.js              |  8 ++--
 lib/dest/write-contents/write-buffer.js       |  4 +-
 lib/dest/write-contents/write-dir.js          | 20 ++++-----
 .../write-contents/write-symbolic-link.js     |  6 +--
 lib/file-operations.js                        | 37 ++++++++--------
 lib/filter-since.js                           | 14 ++++---
 lib/prepare-write.js                          | 14 ++++---
 lib/sink.js                                   |  4 +-
 lib/src/get-contents/index.js                 | 13 +++---
 lib/src/get-contents/read-buffer.js           | 12 +++---
 lib/src/get-contents/read-dir.js              |  2 +-
 lib/src/get-contents/read-stream.js           |  2 +-
 lib/src/get-contents/read-symbolic-link.js    | 12 +++---
 lib/src/wrap-with-vinyl-file.js               | 42 ++++++++++---------
 lib/symlink/index.js                          | 41 +++++++++++++-----
 test/default-value.js                         |  4 +-
 18 files changed, 147 insertions(+), 111 deletions(-)

diff --git a/lib/default-value.js b/lib/default-value.js
index 07eaa0eb..ad80d339 100644
--- a/lib/default-value.js
+++ b/lib/default-value.js
@@ -1,5 +1,8 @@
 'use strict';
 
-module.exports = function defaultValue(defaultValue, value) {
-  return value === null ? defaultValue : value;
-};
+function defaultValue(defVal, value) {
+  // Double equal to support null & undefined
+  return value == null ? defVal : value;
+}
+
+module.exports = defaultValue;
diff --git a/lib/dest/index.js b/lib/dest/index.js
index 73acbd5a..b1c4477a 100644
--- a/lib/dest/index.js
+++ b/lib/dest/index.js
@@ -17,13 +17,15 @@ function dest(outFolder, opt) {
   var sourcemapsOpt = valueOrFunction(
       ['boolean', 'string', 'object'], opt.sourcemaps);
 
-  function saveFile(file, enc, cb) {
-    prepareWrite(outFolder, file, opt, function(err) {
-      if (err) {
-        return cb(err);
+  function saveFile(file, enc, callback) {
+    prepareWrite(outFolder, file, opt, onPrepare);
+
+    function onPrepare(prepareErr) {
+      if (prepareErr) {
+        return callback(prepareErr);
       }
-      writeContents(file, cb);
-    });
+      writeContents(file, callback);
+    }
   }
 
   var saveStream = through2.obj(opt, saveFile);
diff --git a/lib/dest/write-contents/index.js b/lib/dest/write-contents/index.js
index 03d0b66f..46da8ec1 100644
--- a/lib/dest/write-contents/index.js
+++ b/lib/dest/write-contents/index.js
@@ -33,9 +33,9 @@ function writeContents(file, callback) {
 
   // This is invoked by the various writeXxx modules when they've finished
   // writing the contents.
-  function onWritten(err) {
-    if (isErrorFatal(err)) {
-      return callback(err);
+  function onWritten(writeErr) {
+    if (isErrorFatal(writeErr)) {
+      return callback(writeErr);
     }
 
     callback(null, file);
@@ -48,7 +48,7 @@ function writeContents(file, callback) {
 
     if (err.code === 'EEXIST' && file.flag === 'wx') {
       // Handle scenario for file overwrite failures.
-      return false;   // "These aren't the droids you're looking for"
+      return false;
     }
 
     // Otherwise, this is a fatal error
diff --git a/lib/dest/write-contents/write-buffer.js b/lib/dest/write-contents/write-buffer.js
index d4e3083b..502604e1 100644
--- a/lib/dest/write-contents/write-buffer.js
+++ b/lib/dest/write-contents/write-buffer.js
@@ -17,8 +17,8 @@ function writeBuffer(file, onWritten) {
 
     fo.updateMetadata(fd, file, onUpdate);
 
-    function onUpdate(statErr) {
-      fo.closeFd(statErr, fd, onWritten);
+    function onUpdate(updateErr) {
+      fo.closeFd(updateErr, fd, onWritten);
     }
   }
 
diff --git a/lib/dest/write-contents/write-dir.js b/lib/dest/write-contents/write-dir.js
index 04715e3f..bfd973f4 100644
--- a/lib/dest/write-contents/write-dir.js
+++ b/lib/dest/write-contents/write-dir.js
@@ -27,23 +27,23 @@ function writeDir(file, onWritten) {
 
     fo.updateMetadata(fd, file, onUpdate);
 
-    function onUpdate(statErr) {
-      fo.closeFd(statErr, fd, onWritten);
+    function onUpdate(updateErr) {
+      fo.closeFd(updateErr, fd, onWritten);
     }
   }
 
 }
 
 function isInaccessible(err) {
-    if (!err) {
-      return false;
-    }
-
-    if (err.code === 'EACCES') {
-      return true;
-    }
-
+  if (!err) {
     return false;
   }
 
+  if (err.code === 'EACCES') {
+    return true;
+  }
+
+  return false;
+}
+
 module.exports = writeDir;
diff --git a/lib/dest/write-contents/write-symbolic-link.js b/lib/dest/write-contents/write-symbolic-link.js
index d122c9a6..a824ed13 100644
--- a/lib/dest/write-contents/write-symbolic-link.js
+++ b/lib/dest/write-contents/write-symbolic-link.js
@@ -4,9 +4,9 @@ var fs = require('graceful-fs');
 
 function writeSymbolicLink(file, onWritten) {
   // TODO handle symlinks properly
-  fs.symlink(file.symlink, file.path, function(err) {
-    if (isFatalError(err)) {
-      return onWritten(err);
+  fs.symlink(file.symlink, file.path, function(symlinkErr) {
+    if (isFatalError(symlinkErr)) {
+      return onWritten(symlinkErr);
     }
 
     onWritten();
diff --git a/lib/file-operations.js b/lib/file-operations.js
index 64a4fa38..b8fd9073 100644
--- a/lib/file-operations.js
+++ b/lib/file-operations.js
@@ -141,9 +141,9 @@ function updateMetadata(fd, file, callback) {
 
   fs.fstat(fd, onStat);
 
-  function onStat(err, stat) {
-    if (err) {
-      return callback(err);
+  function onStat(statErr, stat) {
+    if (statErr) {
+      return callback(statErr);
     }
 
     // Check if mode needs to be updated
@@ -160,13 +160,13 @@ function updateMetadata(fd, file, callback) {
 
     // Nothing to do
     if (!modeDiff && !timesDiff && !ownerDiff) {
-      return callback(null);
+      return callback();
     }
 
-    // Check access, `futimes` and `fchmod` only work if we own the file,
-    // or if we are effectively root.
+    // Check access, `futimes`, `fchmod` & `fchown` only work if we own
+    // the file, or if we are effectively root (`fchown` only when root).
     if (!isOwner(stat)) {
-      return callback(null);
+      return callback();
     }
 
     if (modeDiff) {
@@ -196,7 +196,7 @@ function updateMetadata(fd, file, callback) {
       }
     }
 
-    function times(fchmodErr) {
+    function times(propagatedErr) {
       fs.futimes(fd, timesDiff.atime, timesDiff.mtime, onFutimes);
 
       function onFutimes(futimesErr) {
@@ -205,13 +205,13 @@ function updateMetadata(fd, file, callback) {
           file.stat.mtime = timesDiff.mtime;
         }
         if (ownerDiff) {
-          return owner(fchmodErr || futimesErr);
+          return owner(propagatedErr || futimesErr);
         }
-        callback(fchmodErr || futimesErr);
+        callback(propagatedErr || futimesErr);
       }
     }
 
-    function owner(earlierErr) {
+    function owner(propagatedErr) {
       fs.fchown(fd, ownerDiff.uid, ownerDiff.gid, onFchown);
 
       function onFchown(fchownErr) {
@@ -219,7 +219,7 @@ function updateMetadata(fd, file, callback) {
           file.stat.uid = ownerDiff.uid;
           file.stat.gid = ownerDiff.gid;
         }
-        callback(earlierErr || fchownErr);
+        callback(propagatedErr || fchownErr);
       }
     }
   }
@@ -237,8 +237,7 @@ function writeFile(filepath, data, options, callback) {
   }
 
   if (!Buffer.isBuffer(data)) {
-    callback(new TypeError('Data must be a Buffer'));
-    return;
+    return callback(new TypeError('Data must be a Buffer'));
   }
 
   if (!options) {
@@ -252,15 +251,15 @@ function writeFile(filepath, data, options, callback) {
 
   fs.open(filepath, flag, mode, onOpen);
 
-  function onOpen(err, fd) {
-    if (err) {
-      return onComplete(err);
+  function onOpen(openErr, fd) {
+    if (openErr) {
+      return onComplete(openErr);
     }
 
     fs.write(fd, data, 0, data.length, position, onComplete);
 
-    function onComplete(err) {
-      callback(err, fd);
+    function onComplete(writeErr) {
+      callback(writeErr, fd);
     }
   }
 }
diff --git a/lib/filter-since.js b/lib/filter-since.js
index 234b5898..31973a2b 100644
--- a/lib/filter-since.js
+++ b/lib/filter-since.js
@@ -2,15 +2,17 @@
 
 var filter = require('through2-filter');
 
-module.exports = function(d) {
-  var isValid = typeof d === 'number' ||
-    d instanceof Number ||
-    d instanceof Date;
+function filterSince(date) {
+  var isValid = typeof date === 'number' ||
+    date instanceof Number ||
+    date instanceof Date;
 
   if (!isValid) {
-    throw new Error('expected since option to be a date or a number');
+    throw new Error('expected since option to be a date or timestamp');
   }
   return filter.obj(function(file) {
-    return file.stat && file.stat.mtime > d;
+    return file.stat && file.stat.mtime > date;
   });
 };
+
+module.exports = filterSince;
diff --git a/lib/prepare-write.js b/lib/prepare-write.js
index 5b2ecc19..dd9b03f5 100644
--- a/lib/prepare-write.js
+++ b/lib/prepare-write.js
@@ -12,7 +12,7 @@ var boolean = valueOrFunction.boolean;
 var number = valueOrFunction.number;
 var string = valueOrFunction.string;
 
-function prepareWrite(outFolder, file, opt, cb) {
+function prepareWrite(outFolder, file, opt, callback) {
   if (!opt) {
     opt = {};
   }
@@ -47,12 +47,14 @@ function prepareWrite(outFolder, file, opt, cb) {
   file.base = basePath;
   file.path = writePath;
 
-  fo.mkdirp(writeFolder, options.dirMode, function(err) {
-    if (err) {
-      return cb(err);
+  fo.mkdirp(writeFolder, options.dirMode, onMkdirp);
+
+  function onMkdirp(mkdirpErr) {
+    if (mkdirpErr) {
+      return callback(mkdirpErr);
     }
-    cb(null);
-  });
+    callback();
+  }
 }
 
 module.exports = prepareWrite;
diff --git a/lib/sink.js b/lib/sink.js
index cfab4698..acb63b3d 100644
--- a/lib/sink.js
+++ b/lib/sink.js
@@ -14,8 +14,8 @@ function sink(stream) {
   var sinkAdded = false;
   var sinkStream = new Writable({
     objectMode: true,
-    write: function(file, enc, cb) {
-      cb();
+    write: function(file, enc, callback) {
+      callback();
     },
   });
 
diff --git a/lib/src/get-contents/index.js b/lib/src/get-contents/index.js
index c00eb418..b0172740 100644
--- a/lib/src/get-contents/index.js
+++ b/lib/src/get-contents/index.js
@@ -7,7 +7,8 @@ var readBuffer = require('./read-buffer');
 var readSymbolicLink = require('./read-symbolic-link');
 
 function getContents(opt) {
-  return through2.obj(opt, function(file, enc, callback) {
+
+  function readFile(file, enc, callback) {
     // Don't fail to read a directory
     if (file.isDirectory()) {
       return readDir(file, opt, onRead);
@@ -28,12 +29,12 @@ function getContents(opt) {
 
     // This is invoked by the various readXxx modules when they've finished
     // reading the contents.
-    function onRead(err) {
-      callback(err, file);
+    function onRead(readErr) {
+      callback(readErr, file);
     }
-  });
-}
-
+  }
 
+  return through2.obj(opt, readFile);
+}
 
 module.exports = getContents;
diff --git a/lib/src/get-contents/read-buffer.js b/lib/src/get-contents/read-buffer.js
index 13ac02ea..b67fc794 100644
--- a/lib/src/get-contents/read-buffer.js
+++ b/lib/src/get-contents/read-buffer.js
@@ -4,9 +4,11 @@ var fs = require('graceful-fs');
 var stripBom = require('strip-bom');
 
 function bufferFile(file, opt, onRead) {
-  fs.readFile(file.path, function(err, data) {
-    if (err) {
-      return onRead(err);
+  fs.readFile(file.path, onReadFile);
+
+  function onReadFile(readErr, data) {
+    if (readErr) {
+      return onRead(readErr);
     }
 
     if (opt.stripBOM) {
@@ -15,8 +17,8 @@ function bufferFile(file, opt, onRead) {
       file.contents = data;
     }
 
-    onRead(null);
-  });
+    onRead();
+  }
 }
 
 module.exports = bufferFile;
diff --git a/lib/src/get-contents/read-dir.js b/lib/src/get-contents/read-dir.js
index 38296f48..63cc706b 100644
--- a/lib/src/get-contents/read-dir.js
+++ b/lib/src/get-contents/read-dir.js
@@ -2,7 +2,7 @@
 
 function readDir(file, opt, onRead) {
   // Do nothing for now
-  onRead(null);
+  onRead();
 }
 
 module.exports = readDir;
diff --git a/lib/src/get-contents/read-stream.js b/lib/src/get-contents/read-stream.js
index 6c44611b..7e8f2b20 100644
--- a/lib/src/get-contents/read-stream.js
+++ b/lib/src/get-contents/read-stream.js
@@ -20,7 +20,7 @@ function streamFile(file, opt, onRead) {
     file.contents = file.contents.pipe(stripBom());
   }
 
-  onRead(null);
+  onRead();
 }
 
 module.exports = streamFile;
diff --git a/lib/src/get-contents/read-symbolic-link.js b/lib/src/get-contents/read-symbolic-link.js
index beb27c6a..89dc2f01 100644
--- a/lib/src/get-contents/read-symbolic-link.js
+++ b/lib/src/get-contents/read-symbolic-link.js
@@ -3,16 +3,18 @@
 var fs = require('graceful-fs');
 
 function readLink(file, opt, onRead) {
-  fs.readlink(file.path, function(err, target) {
-    if (err) {
-      return onRead(err);
+  fs.readlink(file.path, onReadlink);
+
+  function onReadlink(readErr, target) {
+    if (readErr) {
+      return onRead(readErr);
     }
 
     // Store the link target path
     file.symlink = target;
 
-    return onRead(null);
-  });
+    onRead();
+  }
 }
 
 module.exports = readLink;
diff --git a/lib/src/wrap-with-vinyl-file.js b/lib/src/wrap-with-vinyl-file.js
index 5e6290e5..c52b1e9d 100644
--- a/lib/src/wrap-with-vinyl-file.js
+++ b/lib/src/wrap-with-vinyl-file.js
@@ -7,10 +7,12 @@ var File = require('vinyl');
 function wrapWithVinylFile(options) {
 
   // A stat property is exposed on file objects as a (wanted) side effect
-  function resolveFile(globFile, enc, cb) {
-    fs.lstat(globFile.path, function(err, stat) {
-      if (err) {
-        return cb(err);
+  function resolveFile(globFile, enc, callback) {
+    fs.lstat(globFile.path, onStat);
+
+    function onStat(statErr, stat) {
+      if (statErr) {
+        return callback(statErr);
       }
 
       globFile.stat = stat;
@@ -24,25 +26,27 @@ function wrapWithVinylFile(options) {
           // Updated file stats will tell getContents() to actually read it.
           vinylFile.path = globFile.originalSymlinkPath;
         }
-        return cb(null, vinylFile);
+        return callback(null, vinylFile);
       }
 
-      fs.realpath(globFile.path, function(err, filePath) {
-        if (err) {
-          return cb(err);
-        }
+      fs.realpath(globFile.path, onRealpath);
+    }
 
-        if (!globFile.originalSymlinkPath) {
-          // Store the original symlink path before the recursive call
-          // to later rewrite it back.
-          globFile.originalSymlinkPath = globFile.path;
-        }
-        globFile.path = filePath;
+    function onRealpath(realpathErr, filePath) {
+      if (realpathErr) {
+        return callback(realpathErr);
+      }
+
+      if (!globFile.originalSymlinkPath) {
+        // Store the original symlink path before the recursive call
+        // to later rewrite it back.
+        globFile.originalSymlinkPath = globFile.path;
+      }
+      globFile.path = filePath;
 
-        // Recurse to get real file stat
-        resolveFile(globFile, enc, cb);
-      });
-    });
+      // Recurse to get real file stat
+      resolveFile(globFile, enc, callback);
+    }
   }
 
   return through2.obj(options, resolveFile);
diff --git a/lib/symlink/index.js b/lib/symlink/index.js
index e2b0f6fa..b046b297 100644
--- a/lib/symlink/index.js
+++ b/lib/symlink/index.js
@@ -15,14 +15,16 @@ function symlink(outFolder, opt) {
     opt = {};
   }
 
-  function linkFile(file, enc, cb) {
+  function linkFile(file, enc, callback) {
     var srcPath = file.path;
     var symType = (file.isDirectory() ? 'dir' : 'file');
     var isRelative = defaultValue(false, boolean(opt.relative, file));
 
-    prepareWrite(outFolder, file, opt, function(err) {
-      if (err) {
-        return cb(err);
+    prepareWrite(outFolder, file, opt, onPrepare);
+
+    function onPrepare(prepareErr) {
+      if (prepareErr) {
+        return callback(prepareErr);
       }
 
       // This is done inside prepareWrite to use the adjusted file.base property
@@ -30,13 +32,15 @@ function symlink(outFolder, opt) {
         srcPath = path.relative(file.base, srcPath);
       }
 
-      fs.symlink(srcPath, file.path, symType, function(err) {
-        if (err && err.code !== 'EEXIST') {
-          return cb(err);
-        }
-        cb(null, file);
-      });
-    });
+      fs.symlink(srcPath, file.path, symType, onSymlink);
+    }
+
+    function onSymlink(symlinkErr) {
+      if (isErrorFatal(symlinkErr)) {
+        return callback(symlinkErr);
+      }
+      callback(null, file);
+    }
   }
 
   var stream = through2.obj(opt, linkFile);
@@ -45,4 +49,19 @@ function symlink(outFolder, opt) {
   return stream;
 }
 
+function isErrorFatal(err) {
+  if (!err) {
+    return false;
+  }
+
+  // TODO: should we check file.flag like .dest()?
+  if (err.code === 'EEXIST') {
+    // Handle scenario for file overwrite failures.
+    return false;
+  }
+
+  // Otherwise, this is a fatal error
+  return true;
+}
+
 module.exports = symlink;
diff --git a/test/default-value.js b/test/default-value.js
index cabd036d..dde0a601 100644
--- a/test/default-value.js
+++ b/test/default-value.js
@@ -10,8 +10,8 @@ describe('defaultVaule', function() {
     expect(defaultValue('defaultValue', 1)).toBe(1);
   });
 
-  it('returns the value if the value is undefined', function() {
-    expect(defaultValue('defaultValue', undefined)).toBe(undefined);
+  it('returns the default value if the value is undefined', function() {
+    expect(defaultValue('defaultValue', undefined)).toBe('defaultValue');
   });
 
   it('returns the default value if the value is null', function() {