Node-fs-extra: moveSync support

[PeterDaveHello]

Would you like to have moveSync just like copy and copySync? Thanks :smile:

[RyanZim]

I wouldn't be opposed, @jprichardson ?

[jprichardson]

I wouldn't be opposed, @jprichardson ?

Would definitely love this!

Are you up for a PR @PeterDaveHello?

[PeterDaveHello]

I'm afraid I'm not sure if I have enough time to implement this, though I know it's not that huge, but I'm not familiar with fs-extra codes, so anyone want to do it please feel free to do, thanks :smile:

[manidlou]

Hey guys, I've been using the fs-extra for a while and wanted to thank you all for this great and reliable module. In the meantime, I've been reading the source code for the past few days and if you don't mind, I'd like to help improve the fs-extra as much as I can, and please I want you to correct me wherever I am wrong.

Isn't true that the move functionality is essentially copying the src to dest and removing the src?

Regarding the moveSync functionality, the easiest solution that I've found is basically using nothing but combination of the fs-extra's own copySync and removeSync.

I've tried with this,

const cpSync = require('../copy-sync').copySync
const rmSync = require('../remove').removeSync

function mvSync (source, dest, options) {
  cpSync(source, dest, options)
  rmSync(source)
}

and I tested for various cases for both file and directory and got the expected results.

[PeterDaveHello]

@mawni thanks for your help, maybe you can consider to directly send your pull request?

[PeterDaveHello]

@mawni btw, IMO, the move operation is not the same as the combination of copy + remove in the filesystem level, use the combination could also have performance concerns.

[manidlou]

@PeterDaveHello thanks for a great note. Please let me see what I can do and I'll send a pull request then.

[jprichardson]

@mawni thanks. Both @RyanZim and I would love your help. One of the tricky things about moving files is moving them across devices in Windows. So before you submit a PR, please test in Windows moving across drives.

[manidlou]

In the lib/move/index.js says "needs cleanup" https://github.com/jprichardson/node-fs-extra/blob/master/lib/move/index.js#L7.

What are the main concerns here?

[jprichardson]

What are the main concerns here?

At a quick glance, I'm thinking the functions just seemed really long and could be broken down. Probably not a huge priority, but if you feel like cleaning it up, go for it!

[manidlou]

Sure thing. The main reason that I asked that was since I am working on moveSync(), for the sake of consistency and simplicity, I am following the same pattern as move() (of course but sync) and wanted to make sure if it is a right thing to do.

So if the overall algorithm is fine, then I can cleanup move() and then finish moveSync() along with its tests afterwards.

Thoughts?

[manidlou]

Why is this test skipped?

https://github.com/jprichardson/node-fs-extra/blob/master/lib/move/__tests__/move.test.js#L310

[RyanZim]

@manidlou IIRC, the test is skipped because it's a known issue. The test should promptly fail with a smart error message, but it doesn't. Links to https://github.com/jprichardson/node-fs-extra/issues/83.

[manidlou]

shouldMkdirp option in move still exists in the code,

https://github.com/jprichardson/node-fs-extra/blob/master/lib/move/index.js#L21

but currently there is nothing about it in move docs.

https://github.com/jprichardson/node-fs-extra/blob/master/docs/move.md

Do we want to keep the option or it needs to be removed form the source code to be consistent with its docs?

[RyanZim]

@manidlou There are a couple of similar options thought fs-extra. Most of them:

• Are unstable, or
• Don't have tests, or
• Are part of a WIP of adding additional functionality to fs-extra, or
• Were imported from another lib, and we're not sure if we should support the option or remove it.

Since they aren't documented, they are not subject to semver. Therefore, we can remove or change them at any time.

If/When we document them, we will need to ensure they follow semver (which requires good tests).

---

Each undocumented option is a case-by-case basis, ask @jprichardson about this one. Personally, I would vote for removal, but others may differ.

[jprichardson]

Personally, I would vote for removal, but others may differ.

Vote for removal of what?

[RyanZim]

@jprichardson

Vote for removal of what?

The undocumented shouldMkdirp option for move().

[jprichardson]

The undocumented shouldMkdirp option for move().

Agreed, I don't know why a person wouldn't want it.