AvianFlu / ncp

Asynchronous recursive file copying with Node.js.
MIT License
680 stars 103 forks source link

NCP usage seems broken for any real life needs #82

Open lietu opened 9 years ago

lietu commented 9 years ago

Normal people when they think of issuing a command: "copy folder X into folder Y" expect that the result they get is a folder under Y called X with all the contents of folder X in it.

NCP breaks this in two ways:

  1. You end up with folder Y with contents of folder X in it
  2. If you have more than Z items in the folder it just stops copying, there seems to be no way to disable this limit.

Now, try this out in a shell:

mkdir foo
mkdir bar
touch foo/file
cp -r foo bar

What do you end up with?

Yes, you will have a bar/foo/file. This is the expected behavior from anything that copies things recursively.

If you execute instead:

node
require("ncp").ncp("foo", "bar")

You end up with bar/file, you might expect this result with "foo/*" as the first argument, or something similar, but this is just bad.

felicienfrancois commented 9 years ago

First you're not right, the result of the shell command cp -r foo bar do different things depending on dest dir exists or not. The default behavior (when the dir does not exists) is to ends up with bar/file, just like ncp.

Then, the command cp -r foo bar is misleading and should not be used. Instead use cp -r foo bar/ if you want to get foo inside bar (ncp("foo", "bar/")) or cp -r foo/* bar/ if you want to get foo files inside bar (ncp("foo", "bar"))

Then, the difference between ncp and cp is welcomed because unlike you said, it allows all possible usage for any real life needs: If ncp had the same behavior than cp, it would be impossible to do cp -r foo/* bar without supporting glob pattern (usage of *) or knowing parent dir path.

To conclude, this is not ncp which is wierd but cp. ncp behavior is clear, logical and practical (close to mv in the shell) cp behavior is missleading when there is no trailing slash (depends on folder existence), requires usage of glob pattern for half of the cases, and is not coherent with other commands (example: mv).

felicienfrancois commented 9 years ago

Additionally "Normal people" should understand the following:

The slash means inside, this is universal and that's the most important to catch. The behavior of cp when the dest folder exists IS A BUG (maybe a wanted bug but a bug) which workaround is to use glob pattern.

lietu commented 9 years ago

Ah, well that does make sense .. Then I guess there is one remaining issue: documentation.

This doesn't seem to be mentioned in the README.md, and there's no code documentation that I can see.

It would be a good idea to at least add this information on the project README, and it would be nice to also have JSDoc

felicienfrancois commented 9 years ago

Yes you're right. It should be documented.