From 6098de46d4983379ee44f6831c4ccd030fb68d16 Mon Sep 17 00:00:00 2001 From: Michael Mayer Date: Sun, 22 Feb 2015 21:39:42 +0100 Subject: [PATCH 1/2] fs: fix link and symlink error messages Currently, the error messages for missing arguments of symlink, link, symlinkSync and linkSync are swapped. Additionally they are named inconsistently all over the sources and the docs. In addition, #8920 shows that the meaning of `srcpath` and `dstpath` isn't clear and the docs aren't comprehensive enough. This PR changes `srcpath` to `target` and `dstpath` to `path` in the docs and the sources and make the code more consistent. --- doc/api/fs.markdown | 24 ++++++---- lib/fs.js | 41 ++++++++--------- src/node_file.cc | 16 +++---- test/simple/test-fs-error-messages.js | 64 +++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 38 deletions(-) diff --git a/doc/api/fs.markdown b/doc/api/fs.markdown index a1727778e82..41e93c15ad9 100644 --- a/doc/api/fs.markdown +++ b/doc/api/fs.markdown @@ -200,25 +200,33 @@ Synchronous lstat(2). Returns an instance of `fs.Stats`. Synchronous fstat(2). Returns an instance of `fs.Stats`. -## fs.link(srcpath, dstpath, callback) +## fs.link(target, path, callback) Asynchronous link(2). No arguments other than a possible exception are given to the completion callback. -## fs.linkSync(srcpath, dstpath) +## fs.linkSync(target, path) Synchronous link(2). -## fs.symlink(srcpath, dstpath[, type], callback) +## fs.symlink(target, path[, type], callback) -Asynchronous symlink(2). No arguments other than a possible exception are given -to the completion callback. +Asynchronous symlink(2). Creates a symbolic link at `path`, which links to `target`. +No arguments other than a possible exception are given to the completion callback. The `type` argument can be set to `'dir'`, `'file'`, or `'junction'` (default is `'file'`) and is only available on Windows (ignored on other platforms). -Note that Windows junction points require the destination path to be absolute. When using -`'junction'`, the `destination` argument will automatically be normalized to absolute path. +Note that Windows junction points require the target path to be absolute. When using +`'junction'`, the `target` argument will automatically be normalized to absolute path. + +Example: + + var fs = require('fs'); + fs.symlink('/', 'a.symlink.to.root', function(err){ + if (err) throw err; + console.log(fs.realpathSync('a.symlink.to.root')); // output: '/' + }); -## fs.symlinkSync(srcpath, dstpath[, type]) +## fs.symlinkSync(target, path[, type]) Synchronous symlink(2). diff --git a/lib/fs.js b/lib/fs.js index a22da415920..34c8cea829e 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -827,51 +827,48 @@ function preprocessSymlinkDestination(path, type) { } } -fs.symlink = function(destination, path, type_, callback) { - var type = (util.isString(type_) ? type_ : null); - var callback = makeCallback(arguments[arguments.length - 1]); - - if (!nullCheck(destination, callback)) return; +fs.symlink = function(target, path, type, callback) { + callback = makeCallback(arguments[arguments.length - 1]); + if (!nullCheck(target, callback)) return; if (!nullCheck(path, callback)) return; + type = (util.isString(type) ? type : null); var req = new FSReqWrap(); req.oncomplete = callback; - binding.symlink(preprocessSymlinkDestination(destination, type), + binding.symlink(preprocessSymlinkDestination(target, type), pathModule._makeLong(path), type, req); }; -fs.symlinkSync = function(destination, path, type) { - type = (util.isString(type) ? type : null); - - nullCheck(destination); +fs.symlinkSync = function(target, path, type) { + nullCheck(target); nullCheck(path); - - return binding.symlink(preprocessSymlinkDestination(destination, type), + type = (util.isString(type) ? type : null); + return binding.symlink(preprocessSymlinkDestination(target, type), pathModule._makeLong(path), type); }; -fs.link = function(srcpath, dstpath, callback) { +fs.link = function(target, path, callback) { callback = makeCallback(callback); - if (!nullCheck(srcpath, callback)) return; - if (!nullCheck(dstpath, callback)) return; + if (!nullCheck(target, callback)) return; + if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; - binding.link(pathModule._makeLong(srcpath), - pathModule._makeLong(dstpath), + binding.link(pathModule._makeLong(target), + pathModule._makeLong(path), req); }; -fs.linkSync = function(srcpath, dstpath) { - nullCheck(srcpath); - nullCheck(dstpath); - return binding.link(pathModule._makeLong(srcpath), - pathModule._makeLong(dstpath)); +fs.linkSync = function(target, path) { + nullCheck(target); + nullCheck(path); + return binding.link(pathModule._makeLong(target), + pathModule._makeLong(path)); }; fs.unlink = function(path, callback) { diff --git a/src/node_file.cc b/src/node_file.cc index e04c4c2ddde..d6292768709 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -527,13 +527,13 @@ static void Symlink(const FunctionCallbackInfo& args) { int len = args.Length(); if (len < 1) - return TYPE_ERROR("dest path required"); + return TYPE_ERROR("target required"); if (len < 2) - return TYPE_ERROR("src path required"); + return TYPE_ERROR("path required"); if (!args[0]->IsString()) - return TYPE_ERROR("dest path must be a string"); + return TYPE_ERROR("target must be a string"); if (!args[1]->IsString()) - return TYPE_ERROR("src path must be a string"); + return TYPE_ERROR("path must be a string"); node::Utf8Value dest(args[0]); node::Utf8Value path(args[1]); @@ -563,13 +563,13 @@ static void Link(const FunctionCallbackInfo& args) { int len = args.Length(); if (len < 1) - return TYPE_ERROR("dest path required"); + return TYPE_ERROR("target required"); if (len < 2) - return TYPE_ERROR("src path required"); + return TYPE_ERROR("path required"); if (!args[0]->IsString()) - return TYPE_ERROR("dest path must be a string"); + return TYPE_ERROR("target must be a string"); if (!args[1]->IsString()) - return TYPE_ERROR("src path must be a string"); + return TYPE_ERROR("path must be a string"); node::Utf8Value orig_path(args[0]); node::Utf8Value new_path(args[1]); diff --git a/test/simple/test-fs-error-messages.js b/test/simple/test-fs-error-messages.js index 16b5dd92b3f..a66caa3b98a 100644 --- a/test/simple/test-fs-error-messages.js +++ b/test/simple/test-fs-error-messages.js @@ -141,6 +141,70 @@ try { assert.ok(0 <= err.message.indexOf(fn)); } +try { + ++expected; + fs.linkSync(); +} catch (err) { + errors.push('link'); + assert.ok(0 <= err.message.indexOf('target')); +} + +try { + ++expected; + fs.symlinkSync(); +} catch (err) { + errors.push('link'); + assert.ok(0 <= err.message.indexOf('target')); +} + +try { + ++expected; + fs.linkSync({}); +} catch (err) { + errors.push('link'); + assert.ok(0 <= err.message.indexOf('target')); +} + +try { + ++expected; + fs.symlinkSync({}); +} catch (err) { + errors.push('link'); + assert.ok(0 <= err.message.indexOf('target')); +} + +try { + ++expected; + fs.linkSync(existingFile); +} catch (err) { + errors.push('link'); + assert.ok(0 <= err.message.indexOf('path')); +} + +try { + ++expected; + fs.symlinkSync(existingFile); +} catch (err) { + errors.push('link'); + assert.ok(0 <= err.message.indexOf('path')); +} + +try { + ++expected; + fs.linkSync(existingFile, {}); +} catch (err) { + errors.push('link'); + assert.ok(0 <= err.message.indexOf('path')); +} + +try { + ++expected; + fs.symlinkSync(existingFile, {}); +} catch (err) { + errors.push('link'); + assert.ok(0 <= err.message.indexOf('path')); +} + try { ++expected; fs.linkSync(fn, 'foo'); From d6156b16c7262d944b782e4461ecd7eee23c506b Mon Sep 17 00:00:00 2001 From: Michael Mayer Date: Mon, 23 Feb 2015 10:47:39 +0100 Subject: [PATCH 2/2] missing substitutions in src/node_file.cc --- src/node_file.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index d6292768709..924e469c13b 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -535,7 +535,7 @@ static void Symlink(const FunctionCallbackInfo& args) { if (!args[1]->IsString()) return TYPE_ERROR("path must be a string"); - node::Utf8Value dest(args[0]); + node::Utf8Value target(args[0]); node::Utf8Value path(args[1]); int flags = 0; @@ -551,9 +551,9 @@ static void Symlink(const FunctionCallbackInfo& args) { } if (args[3]->IsObject()) { - ASYNC_DEST_CALL(symlink, args[3], *path, *dest, *path, flags) + ASYNC_DEST_CALL(symlink, args[3], *path, *target, *path, flags) } else { - SYNC_DEST_CALL(symlink, *dest, *path, *dest, *path, flags) + SYNC_DEST_CALL(symlink, *target, *path, *target, *path, flags) } } @@ -571,13 +571,13 @@ static void Link(const FunctionCallbackInfo& args) { if (!args[1]->IsString()) return TYPE_ERROR("path must be a string"); - node::Utf8Value orig_path(args[0]); - node::Utf8Value new_path(args[1]); + node::Utf8Value target(args[0]); + node::Utf8Value path(args[1]); if (args[2]->IsObject()) { - ASYNC_DEST_CALL(link, args[2], *new_path, *orig_path, *new_path) + ASYNC_DEST_CALL(link, args[2], *path, *target, *path) } else { - SYNC_DEST_CALL(link, *orig_path, *new_path, *orig_path, *new_path) + SYNC_DEST_CALL(link, *target, *path, *target, *path) } }