Uh oh!
There was an error while loading. Please reload this page.
path: add type checking for path inputs - #1153
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?
Fishrock123
commented
Mar 14, 2015
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:
functionassertPaths(){[].slice.call(arguments).forEach(function(path,i){if(typeofpath!=='string')thrownewTypeError(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
functionassertPaths(){for(vari=0;i<arguments.length;++i){if(typeofarguments[i]!=='string')thrownewTypeError(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.
brendanashworth
commented
Mar 14, 2015
LGTM |
silverwind
commented
Mar 15, 2015
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.
bnoordhuis
commented
Mar 16, 2015
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>
cjihrig
commented
Mar 16, 2015
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.
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