[BREAKING] move*(): check paths before moving #623
Conversation
|
Just realized I forgot to add the case-insensitive paths test! I add it in a bit. |
|
Ready for review! @jprichardson @RyanZim @JPeer264 |
| @@ -32,9 +33,27 @@ describe('+ move() - prevent moving into itself', () => { | |||
| fs.outputFileSync(path.join(src, FILES[1]), dat1) | |||
| fs.outputFileSync(path.join(src, FILES[2]), dat2) | |||
| fs.outputFileSync(path.join(src, FILES[3]), dat3) | |||
| done() | |||
There was a problem hiding this comment.
Is this needed? Just sync code is executed and an async done() is not really needed right?
There was a problem hiding this comment.
You are right @JPeer264 that's not needed!
| const dest = path.join(src, 'dest') | ||
| return testError(src, dest, done) | ||
| }) | ||
| it(`should move the directory successfully when dest is 'src_dest'`, done => { |
There was a problem hiding this comment.
I don't know if it's cleaner this way (at least new paths can be added with less overhead):
[
'dest',
'dest/src',
'src_dest/src',
...
].forEach(pathName => {
it(`should move directory successfully when dest is ${pathName}`, done => {
const dest = path.join(TEST_DIR, pathName)
return testSuccessDir(src, dest, done)
})
})There was a problem hiding this comment.
@JPeer264 you are right that's another way of doing it, however IMHO it is not a top priority now and we can always refactor tests in the future. (copy() also has those tests)
|
@RyanZim ping?! |
|
@manidlou If you wrote it, and @JPeer264 & @jprichardson both approve it, I'm good with it; my review isn't needed. Just go ahead and merge, I don't have the bandwidth to properly review it ATM. |
fix #614 and #608.
Added the same paths checking that we use in
copy*()tomove*(). This will avoid unexpected behavior and improve reliability of the function!