Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for dest being a directory when using copy*()? #323

Closed
matatk opened this issue Dec 3, 2016 · 7 comments
Closed

Support for dest being a directory when using copy*()? #323

matatk opened this issue Dec 3, 2016 · 7 comments

Comments

@matatk
Copy link

matatk commented Dec 3, 2016

When using copySync() (and, I assume, copy() too): if src is a file path and dest is a directory path, I would expect that the source file would be copied into the destination directory, with the same name that it had before (i.e. the last part at the end of the src path string).

However, it seems that, instead, copy() attempts to replace the dest directory with the src file.

I have tried {clobber: false} but this simply seems to stop the copy operation from taking place [as per your explanation in #321 I now understand why].

To get it to behave as desired, I have to include the destination filename at the end of the dest path. This means duplicating the destination filename. Sometimes, my code that calls copy() does not know the filename (only the full src path), so it is necessary to make an extra call, to path.basename(), first.

I assume that this behaviour is by design to fit in with how (standard) fs works? However I wonder if you could add a further copy()-like function (or option on the fs-extra version of copy() to support this behaviour?

Thanks for this very helpful module (only just started using it, but it’s already saved me lots of time) :-).

@jprichardson
Copy link
Owner

When using copySync() (and, I assume, copy() too): if src is a file path and dest is a directory path, I would expect that the source file would be copied into the destination directory, with the same name that it had before (i.e. the last part at the end of the src path string).

I understand the perspective here. A lot of people have requested similar functionality. It's my belief that we as programmers should be explicit where possible. As reading code is harder than writing code. fs-extra has chosen to encourage this by making programmer intent obvious to those who read the code.

I think where this functionality makes sense is in the case of shell programs where terse expressiveness is valued given the longevity of shell scripts.

Hope this makes sense.

Thanks for this very helpful module (only just started using it, but it’s already saved me lots of time) :-).

You're welcome. @RyanZim and I have a lot of great things planned for this module! :)

@RyanZim
Copy link
Collaborator

RyanZim commented Dec 3, 2016

@jprichardson BTW, we should document this fact clearly somewhere, since a lot of people seem to be asking about this.

@matatk
Copy link
Author

matatk commented Dec 4, 2016

@jprichardson: thanks for the thoughtful explanation.

I agree with @RyanZim that a note in the docs would help clarify how it works.

Thanks again, both (and for the very quick replies).

@just-boris
Copy link

It's my belief that we as programmers should be explicit where possible.

IMO, the devs should avoid copy-pasting parts as they cause typos and bugs. For example, I want to copy bunch of files

copySync(`${srcRoot}/angular.js`, `${destRoot}/angular.js`)
copySync(`${srcRoot}/react.js`, `${destRoot}/react.js`)
copySync(`${srcRoot}/vue.js`, `${destRoot}/vue.js`)

Repeating the same file name twice will not help anyone reading my code to understand it better. But there are more chances to make a typo, and then it will be very hard to track down, why file is named wrong at the other end.

That's why I have created 2-line helper that appends file name for me:

const path = require('path')
const { copySync } = require('fs-extra')

function copyFileSync(filePath, destDir) {
    const filename = path.basename(filePath)
    return copySync(filePath, path.join(destDir, filename))
}

Works perfectly! Code is cleaner and there are less places where dev can make a spelling mistake.

copyFileSync(`${srcRoot}/angular.js`, destRoot)
copyFileSync(`${srcRoot}/react.js`, destRoot)
copyFileSync(`${srcRoot}/vue.js`, destRoot)

@RyanZim
Copy link
Collaborator

RyanZim commented Dec 28, 2017

@just-boris you need to rethink how you're writing code. You have 3 lines that are almost the same. Consider something like this instead:

;['angular', 'react', 'vue']
  .forEach(name => fs.copySync(`${srcRoot}/${name}.js`, `${destRoot}/${name}.js`))

@just-boris
Copy link

@RyanZim well this is another option that is preferable sometimes. But my real example is slightly more complicated than I have shared before

copyFileSync(`${srcRoot}/file-one.js`, `${destRoot}/app-one`)
copyFileSync(`${srcRoot}/file-two.js`, `${destRoot}/app-two`)
copyFileSync(`${srcRoot}/file-three.js`, `${destRoot}/app-two`)

There I would go with three copyFileSync calls instad of creating {src, dest} object pairs and iterating over them.

@Retsam
Copy link

Retsam commented Sep 25, 2018

To add another case where it'd be nice to support this: I use a glob to find a bunch of files in different directories, and I'd like to copy them all to the same directory. So I've got something like:

// Not really hardcoded, but returned from a glob query
const input = [
    'absolute/path/to/some/file.img',
    'absolute/path/to/some/other/file.img',
    'absolute/path/to/somewhere/else.img'
];

What I'd like to do is:

input.map(path => fs.copy(filePath, 'ouputDir'))

But instead I've got to do:

input.map(path => fs.copy(filePath, path.join('outputDir', path.basename(filePath))

It's not a showstopper, but I find the first hypothetical snippet a lot more readable.

Repository owner locked and limited conversation to collaborators Sep 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants