search

ryanmcgrath / wrench-js

Recursive file operations in Node.js
MIT License
435 stars 71 forks source link

Broken argument handling with copyDirRecursive #62

Closed medatech closed 11 years ago

medatech commented 11 years ago

I'm failing to understand how the following code can work in copeDirRecursive:

exports.copyDirRecursive = function copyDirRecursive(srcDir, newDir, opts, clbk) { fs.stat(newDir, function(err, newDirStat){ if(!err) { if(typeof opts !== 'undefined' && typeof opts !== 'function' && opts.forceDelete) return exports.rmdirRecursive(newDir, function(err){ copyDirRecursive.apply(this, arguments); }); else return clbk(new Error('You are trying to delete a directory that already exists. Specify forceDelete in an options object to override this.')); }

if(typeof opts === 'function')
    clbk = opts;

I think the following might be wrong:

1 - If you call it with: wrench.copyDirRecursive(srcDir, newDir, function () {});

The check if opts is a function happens too late because if the new directory exists and opts is a function, then it attempts to invoke clbk which is undefined.

2 - After removing the newDir with: return exports.rmdirRecursive(newDir, function(err){ copyDirRecursive.apply(this, arguments); });

Now if I'm not mistaken, arguments is basically containing [err], whereas it was intended for it to call srcDir, newDir, opts, clbk. Also it might not be intended to use 'this' in the call as that depends on the context of the rmdirRecursive.

3 - Mix of spaces and tab indentation makes some of the nesting difficult to follow in an IDE.

ryanmcgrath commented 11 years ago

It is entirely possible you're right. I've sadly not had the time to clean up the pull requests/issues for this lib though as real life has just sucked it all up - feel free to pull request if you'd like to see it changed sooner though. :(

ryanmcgrath commented 11 years ago

Heeey, so 2 months later I'm circling back to this - I believe it's been more or less fixed up with the pull requests from some other people. Thanks for pointing it out! It's totally my bad for not hitting this sooner.