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

Improve error message upon failed copy(file, dir) #761

Closed
bseib opened this issue Feb 13, 2020 · 3 comments
Closed

Improve error message upon failed copy(file, dir) #761

bseib opened this issue Feb 13, 2020 · 3 comments
Assignees
Milestone

Comments

@bseib
Copy link

@bseib bseib commented Feb 13, 2020

  • Operating System: Win10
  • Node.js version: v10.16.0
  • fs-extra version: 8.1.0

When you attempt to call copy('./some-file', './some-dir') you will get an error message that does not reflect what the docs are telling me not to do. The error message you get is about unlink, and I spent too much time tracking down what was really going on in a recent bit of code.

I now understand from issue #323 that it is intentional that a person should not try to call copy() with a file as the source and a directory as the destination, and I support the reasoning to have people be explicit in their intent. When I read the documentation for copy(), I see that this is explained clearly. But I wasn't the original author of the code and I wasn't the one reading the docs before writing the code. I was the debugger of code that came before me.

I propose that an error message be given that is consistent with the copy() documentation when the first arg is a file and the second arg is a directory. If that combination of arguments is documented to never be allowed, then the error message should reflect that.

@RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Feb 13, 2020

@manidlou Seems doable, what do you think?

@manidlou
Copy link
Collaborator

@manidlou manidlou commented Feb 13, 2020

Yeah I agree! In fact, we have implemented the proper message for the reverse case when src is a directory like copy('./some-dir', './some-file').

https://github.com/jprichardson/node-fs-extra/blob/master/lib/copy/copy.js#L146

Maybe it is better we handle that for both cases when we check the src and dest stats at the beginning.

@RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Apr 1, 2021

Fixed in #779

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants