ryanmcgrath / wrench-js

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

Fixed copyDirRecursive when forceDeleting old files #56

Closed danielholmes closed 11 years ago

danielholmes commented 11 years ago

I think the problem is with function scoping and arguments. The applied call to copyDirRecursive doesn't have the right arguments.

NOTE: I'm pretty new to node and some of the test suite is failing on my machine, so it's possible it's an issue with my setup (Mountain Lion with node via homebrew).

danielholmes commented 11 years ago

Example of how im using it (where dest already exists):

wrench.copyDirRecursive "source", "dest", {
    forceDelete: true
    excludeHiddenUnix: true
    preserveFiles: false
}, callback
ryanmcgrath commented 11 years ago

Mmmm, so I'm interested in this pull request - what was the problem you were experiencing? Forgive me if I'm missing something but it's not clear to me what this solves. ^_^;

danielholmes commented 11 years ago

So if we look at just this part here:

return exports.rmdirRecursive(newDir, function(err){
copyDirRecursive.apply(this, arguments);

arguments refers to the arguments for that anonymous function, since it's the closest "parent" function. So it will have that err argument and any other arguments passed to the rmdirRecursive callback. From what i gather (and what works for me after i change manually), the intention is actually to call copyDirRecursive on this line with the arguments passed to the original copyDirRecursive call.

Let me know if I'm still not making sense and i'll try and illustrate another way. One thing you could confirm for me, is this call to copyDirRecursive meant to be called with the same arguments of the original call to copyDirRecursive?

aogriffiths commented 11 years ago

Would be great to get either this pull (#56) or my pull (#59) merged in. They both fixes an issue with copyDirRecursive that prevents it successfully overwriting if the newDir already exists even if forceDelete is true.

Thanks Adam

ryanmcgrath commented 11 years ago

I'll try to get to these later tonight - personal things are keeping me busy at the moment. None of these have gone unnoticed though.

aogriffiths commented 11 years ago

Excellent. I totally understand!

Adam

On 4 Aug 2013, at 20:22, Ryan McGrath notifications@github.com wrote:

I'll try to get to these later tonight - personal things are keeping me busy at the moment. None of these have gone unnoticed though.

— Reply to this email directly or view it on GitHub.

ryanmcgrath commented 11 years ago

So, I apologize for being months late here - life is a bitch sometimes.

I merged @aogriffiths patch, which from what I gather accomplishes the same thing as this one. I appreciate both pull requests, though - please let me know if I've missed something here. Thanks!

aogriffiths commented 11 years ago

Thanks for the merge. Adam