Add moveSync and its tests #381
Conversation
| try { | ||
| fse.moveSync(src, dest) | ||
| } catch (err) { | ||
| assert.ifError(err) |
There was a problem hiding this comment.
All these try..catch blocks are unnecessary; if moveSync fails, it will automatically fail the test with the error message. Adding a try..catch with an assert.ifError is just redundant.
| done() | ||
| }) | ||
| }) | ||
| */ |
| const contents = fs.readFileSync(dest, 'utf8') | ||
| const expected = /^sonic the hedgehog\r?\n$/ | ||
| assert.ok(contents.match(expected), `${contents} match ${expected}`) | ||
| tearDownMockFs('EXDEV') |
There was a problem hiding this comment.
No need to pass a parameter here, tearDownMockFs doesn't use it. Correct other places where this is done.
| }) | ||
| }) | ||
|
|
||
| describe.skip('> when trying to a move a folder into itself', () => { |
There was a problem hiding this comment.
Because prevent copying into itself is not implemented yet for copySync. But as I am thinking now, that would work only for the cases if we move across devices. But, when src and dest are on the same device, we still should be able to prevent moving into itself. Is that right?
There was a problem hiding this comment.
Yeah, just wanted to make sure the .skip wasn't included by mistake.
|
|
||
| // tested on Linux ubuntu 3.13.0-32-generic #57-Ubuntu SMP i686 i686 GNU/Linux | ||
| // this won't trigger a bug on Mac OS X Yosimite with a USB drive (/Volumes) | ||
| // see issue #108 |
There was a problem hiding this comment.
Does this comment apply to moveSync?
There was a problem hiding this comment.
IDK! @jprichardson?
lib/move-sync/index.js
Outdated
| // most of this code was written by Andrew Kelley | ||
| // licensed under the BSD license: see | ||
| // https://github.com/andrewrk/node-mv/blob/master/package.json | ||
| // This is the sync version that somehow follows the same pattern. |
There was a problem hiding this comment.
I'd say just remove this comment; by now, this is going to barely resemble the original code.
lib/move-sync/index.js
Outdated
| options = options || {} | ||
| const overwrite = options.overwrite || options.clobber || false | ||
|
|
||
| if (path.resolve(src) === path.resolve(dest)) return |
There was a problem hiding this comment.
When src and dest are the same, currently it just return. Which one is better, return or throw an error?
manidlou
force-pushed
the
moveSync-support
branch
from
c6f795d
to
08d7d3a
Compare
|
Added prevent moving into itself and its tests for |
|
@manidlou Do you want this or #374 merged first? (Will cause merge conflicts either way.) @jprichardson Would like you to look over this before merge. |
|
Thanks @manidlou! |
|
I will add |
This should resolves #309.
Manually tested on Ubuntu 16.04 and Windows 10 on virtualbox moved across drives succesfully, cases such as
moveSync('G:\\src', 'E:\\nested\\dest').Like always, please feel free to point out any issues that you can think of 馃槂.