path: add type checking for path inputs#1153
path: add type checking for path inputs#1153cjihrig wants to merge 1 commit intonodejs:v1.xfrom cjihrig:1139
Conversation
This commit adds type checking of path inputs to exported methods in the path module. The exception is _makeLong, which seems to explicitly support any data type.
There was a problem hiding this comment.
This will be a little weird because the error will always be "path" no matter which is missing.
There was a problem hiding this comment.
maybe this should be changed to be like line #328?
|
LGTM other than one comment. |
There was a problem hiding this comment.
Maybe something like this makes it more clear which argument is of wrong type:
function assertPaths() {
[].slice.call(arguments).forEach(function (path, i) {
if (typeof path !== 'string')
throw new TypeError(util.format('Argument %d must be a string.' +
' Received %s', i + 1, util.inspect(path));
});
}assertPaths(from, to);There was a problem hiding this comment.
[].slice.call(arguments) <-- slow
The forEach isn't particularly speedy either.
There was a problem hiding this comment.
True, probably better to keep it the way it is then.
I wish rest parameters were in V8 already 😭
There was a problem hiding this comment.
The fastest way to write it is not that much harder actually ;p
function assertPaths() {
for (var i = 0; i < arguments.length; ++i) {
if (typeof arguments[i] !== 'string')
throw new TypeError(util.format('Argument %d must be a string.' +
' Received %s', i + 1, util.inspect(arguments[i])));
}
}There was a problem hiding this comment.
Thanks, that didn't occur to me of course, because I secretly hate classic for loops. I have to jsperf argument iteration sometimes.
There was a problem hiding this comment.
I'd rather not have this be over-engineered, we're just checking for argument types - anyways, as is, you could see which argument is bad via the stack trace.
|
LGTM |
|
Overall looking fine to me, I hope the perf hit isn't too big on this. |
There was a problem hiding this comment.
Minor style nit: can you put braces around the body here? It doesn't fit on a single line.
|
LGTM at a quick glance. |
This commit adds type checking of path inputs to exported methods in the path module. The exception is _makeLong(), which seems to explicitly support any data type. Fixes: #1139 PR-URL: #1153 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
|
Landed in eb995d6 |
Notable Changes: * path: New type-checking on path.resolve() <#1153> uncovered some edge-cases being relied upon in the wild, most notably path.dirname(undefined). Type-checking has been loosened for path.dirname(), path.basename(), and path.extname(), (Colin Ihrig) <#1216>. * querystring: Internal optimizations in querystring.parse() and querystring.stringify() <#847> prevented Number literals from being properly converted via querystring.escape() <#1208>, exposing a blind-spot in the test suite. The bug and the tests have now been fixed (Jeremiah Senkpiel) <#1213>.
| win32.basename = function(path, ext) { | ||
| assertPath(path); | ||
|
|
||
| if (ext !== undefined && typeof ext !== 'string') |
There was a problem hiding this comment.
This causes breakage when used as an Array.map callback, which will pass in an index for ext.
Noticed because I had:
result = directories.map(path.basename);And upgrading now throws:
>> TypeError: ext must be a string
>> at posix.basename (path.js:550:11)
>> at Array.map (native)
Technically an improper use of basename but still a small reduction in utility.
7 participants
This commit adds type checking of path inputs to exported methods in the
pathmodule. The exception is_makeLong(), which seems to explicitly support any data type.Closes #1139