Skip to content

Commit

Permalink
path: Clean up path.parse/format and add tests
Browse files Browse the repository at this point in the history 
+ Remove unused variable and error triggered if
  that variable isn't the right type
+ Remove unreachable code (checking return value of *SplitPath)
+ Remove unnecessary `|| ''` (because what's on the left of that is
  guaranteed to be a string)
  • Loading branch information
@nwoltman
nwoltman committed Mar 24, 2015
1 parent a71e518 commit 6048b25
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 47 deletions.
54 changes: 14 additions & 40 deletions lib/path.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ function win32SplitPath(filename) {
// Separate device+slash from tail
var result = splitDeviceRe.exec(filename),
device = (result[1] || '') + (result[2] || ''),
tail = result[3] || '';
tail = result[3];
// Split the tail into dir, basename and extension
var result2 = splitTailRe.exec(tail),
dir = result2[1],
Expand Down Expand Up @@ -364,42 +364,31 @@ win32.extname = function(path) {
win32.format = function(pathObject) {
if (!util.isObject(pathObject)) {
throw new TypeError(
"Parameter 'pathObject' must be an object, not " + typeof pathObject

This comment has been minimized.

Sign in to view

Copy link
@misterdjules

misterdjules Mar 24, 2015

The fact that pathObject.root is not used shows an inconsistency between this API's documentation and its implementation. I'm not sure what's the best way to fix it but here are the possibilities I can think of:

  1. Actually make path.format take options.root into account to generate its result and keep the documentation as it is, and actually improve it with more examples of how the root option can be used . This could happen in the v0.12 branch if we consider that it's what was intended from the beginning and that this is what the documentation described. We would need to update the tests too, currently they don't test the value of root on Windows or POSIX.

  2. Remove this code as is done in this change and update the documentation because then path.format would not be strictly "the opposite of path.parse" anymore. This can also be done in v0.12.

Personally, I like 1) better because I feel that it would make sense to write path.format({ root: '/foo/bar', dir: 'other_dir', base: 'somefile.txt'}) and expect to get /foo/bar/other_dir/somefile.txt as a result (same for different roots on win32, including different drives).

This is potentially going to be a long discussion, and one that has nothing to do with the original goal of this PR. So unless the removal of this code has a significant impact on performance, I would suggest tracking that and having this discussion in another PR.

What do you think?

Also including @roryrjb since he's the original author of this API with 2d17193.

This comment has been minimized.

Sign in to view

Copy link
@nwoltman

nwoltman Mar 24, 2015

Author Owner

No problem! I'll put the stuff related to pathObject.root back in and then implement your first suggestion in another PR.
);
}

var root = pathObject.root || '';

if (!util.isString(root)) {
throw new TypeError(
"'pathObject.root' must be a string or undefined, not " +
typeof pathObject.root
"Parameter 'pathObject' must be an object, not " + typeof pathObject
);
}

var dir = pathObject.dir;
var base = pathObject.base || '';
if (dir) {
if (dir[dir.length - 1] === win32.sep) {
return dir + base;
}
return dir + win32.sep + base;
}

return base;
if (!dir) {
return base;
}
if (dir[dir.length - 1] === win32.sep) {
return dir + base;
}
return dir + win32.sep + base;
};


win32.parse = function(pathString) {
if (!util.isString(pathString)) {
throw new TypeError(
"Parameter 'pathString' must be a string, not " + typeof pathString
"Parameter 'pathString' must be a string, not " + typeof pathString
);
}

var allParts = win32SplitPath(pathString);
if (!allParts || allParts.length !== 4) {

This comment has been minimized.

Sign in to view

Copy link
@misterdjules

misterdjules Mar 24, 2015

Same idea as in my other comment. win32SplitPath's implementation could change, and we want to make sure our assumptions about its return values are always valid.
throw new TypeError("Invalid path '" + pathString + "'");
}
return {
root: allParts[0],
dir: allParts[0] + allParts[1].slice(0, -1),
Expand Down Expand Up @@ -571,16 +560,7 @@ posix.extname = function(path) {
posix.format = function(pathObject) {
if (!util.isObject(pathObject)) {
throw new TypeError(
"Parameter 'pathObject' must be an object, not " + typeof pathObject
);
}

var root = pathObject.root || '';

if (!util.isString(root)) {
throw new TypeError(
"'pathObject.root' must be a string or undefined, not " +
typeof pathObject.root
"Parameter 'pathObject' must be an object, not " + typeof pathObject
);
}

Expand All @@ -593,17 +573,11 @@ posix.format = function(pathObject) {
posix.parse = function(pathString) {
if (!util.isString(pathString)) {
throw new TypeError(
"Parameter 'pathString' must be a string, not " + typeof pathString
"Parameter 'pathString' must be a string, not " + typeof pathString
);
}
var allParts = posixSplitPath(pathString);
if (!allParts || allParts.length !== 4) {
throw new TypeError("Invalid path '" + pathString + "'");
}
allParts[1] = allParts[1] || '';
allParts[2] = allParts[2] || '';
allParts[3] = allParts[3] || '';

This comment has been minimized.

Sign in to view

Copy link
@misterdjules

misterdjules Mar 24, 2015

This code is dead today because posixSplitPath is implemented with RegExp.exec, and the regexp it uses makes it always return an array of 4 strings.

But the implementation of posixSplitPath could change (it could use something else than a regexp, or a different number of captures). If it changes, we want to make sure that its return value is what we rely on. So I think we should keep this validation.

How much of an impact does this code have on performance in its current form (before the removal)?

This comment has been minimized.

Sign in to view

Copy link
@nwoltman

nwoltman Mar 25, 2015

Author Owner

The impact on performance is basically negligible.
However, even though you make a smart point about the possibility of the implementation of posixSplitPath (and win32SplitPath) changing, I don't think the source code is the proper place to be safeguarding against such a change. That is what unit/regression tests are for. I believe passing in an empty string passed to path.parse as well as other path functions should be sufficient to catch a breaking change in posixSplitPath and win32SplitPath.

This comment has been minimized.

Sign in to view

Copy link
@nwoltman

nwoltman Mar 25, 2015

Author Owner

But since this doesn't really impact performance, I can bring this up again in a different PR.

This comment has been minimized.

Sign in to view

Copy link
@misterdjules

misterdjules Mar 25, 2015

In my opinion, this validation is equivalent to an assert. Its purpose is not to find defects in the code before it is shipped (which is what, among other things, tests are for, as you said). It is:

  • To document the invariants of this code path.
  • To trigger an error at the point where these invariants are not satisfied, instead of letting the program continue and have it fail later in another way that would be much harder to debug.

This is to me complementary to tests, but still very useful.

var allParts = posixSplitPath(pathString);
return {
root: allParts[0],
dir: allParts[0] + allParts[1].slice(0, -1),
Expand Down
29 changes: 22 additions & 7 deletions test/simple/test-path-parse-format.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@ var winPaths = [
'\\\\server two\\shared folder\\file path.zip',
'\\\\teela\\admin$\\system32',
'\\\\?\\UNC\\server\\share'
];

var winSpecialCaseFormatTests = [
[{dir: 'some\\dir'}, 'some\\dir\\'],
[{base: 'index.html'}, 'index.html'],
[{}, '']
];

var unixPaths = [
Expand All @@ -50,25 +55,30 @@ var unixPaths = [
'C:\\foo'
];

var unixSpecialCaseFormatTests = [
[{dir: 'some/dir'}, 'some/dir/'],
[{base: 'index.html'}, 'index.html'],
[{}, '']
];

var errors = [
{method: 'parse', input: [null], message: /Parameter 'pathString' must be a string, not/},
{method: 'parse', input: [{}], message: /Parameter 'pathString' must be a string, not object/},
{method: 'parse', input: [true], message: /Parameter 'pathString' must be a string, not boolean/},
{method: 'parse', input: [1], message: /Parameter 'pathString' must be a string, not number/},
{method: 'parse', input: [], message: /Parameter 'pathString' must be a string, not undefined/},
// {method: 'parse', input: [''], message: /Invalid path/}, // omitted because it's hard to trigger!
{method: 'format', input: [null], message: /Parameter 'pathObject' must be an object, not/},
{method: 'format', input: [''], message: /Parameter 'pathObject' must be an object, not string/},
{method: 'format', input: [true], message: /Parameter 'pathObject' must be an object, not boolean/},
{method: 'format', input: [1], message: /Parameter 'pathObject' must be an object, not number/},
{method: 'format', input: [{root: true}], message: /'pathObject.root' must be a string or undefined, not boolean/},

This comment has been minimized.

Sign in to view

Copy link
@misterdjules

misterdjules Mar 24, 2015

Same as my comment above for removing the tests of the rootoption.
{method: 'format', input: [{root: 12}], message: /'pathObject.root' must be a string or undefined, not number/},
];

check(path.win32, winPaths);
check(path.posix, unixPaths);
checkParseFormat(path.win32, winPaths);
checkParseFormat(path.posix, unixPaths);
checkErrors(path.win32);
checkErrors(path.posix);
checkFormat(path.win32, winSpecialCaseFormatTests);
checkFormat(path.posix, unixSpecialCaseFormatTests);

function checkErrors(path) {
errors.forEach(function(errorCase) {
Expand All @@ -87,8 +97,7 @@ function checkErrors(path) {
});
}


function check(path, paths) {
function checkParseFormat(path, paths) {
paths.forEach(function(element, index, array) {
var output = path.parse(element);
assert.strictEqual(path.format(output), element);
Expand All @@ -97,3 +106,9 @@ function check(path, paths) {
assert.strictEqual(output.ext, path.extname(element));
});
}

function checkFormat(path, testCases) {
testCases.forEach(function(testCase) {
assert.strictEqual(path.format(testCase[0]), testCase[1]);
});
}

0 comments on commit 6048b25

Please sign in to comment.