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

BREAKING: Do not error when copy destination exists & clobber: false #330

Merged
merged 1 commit into from
Dec 29, 2016

Conversation

RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Dec 24, 2016

Add errorOnExist option for users that want an error

Resolves #321

@jprichardson What do you think of the error message?

@RyanZim RyanZim added this to the 2.0.0 milestone Dec 24, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 85.407% when pulling c09ca54 on copy-clobber into 5f319b6 on master.

@RyanZim
Copy link
Collaborator Author

RyanZim commented Dec 24, 2016

Hold off on this, copySync needs edited too.

Add errorOnExist option for users that want an error

Resolves #321
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 85.391% when pulling 3fa83d4 on copy-clobber into 5f319b6 on master.

@RyanZim
Copy link
Collaborator Author

RyanZim commented Dec 24, 2016

copySync updated.

@RyanZim
Copy link
Collaborator Author

RyanZim commented Dec 28, 2016

@jprichardson I think this is merge-ready, just want you to approve the error message first.

@jprichardson jprichardson merged commit 2dd4c0e into master Dec 29, 2016
@jprichardson
Copy link
Owner

Awesome, thanks!

@ghost
Copy link

ghost commented Aug 10, 2017

@RyanZim

What do you think of the error message?

Just a thought - do you think throwing the same style error messages as fs returns would be better suited since this module is meant as a drop-in replacement?

Meaning that instead of ${path} already exists one might throw an EEXIST error?

@RyanZim
Copy link
Collaborator Author

RyanZim commented Aug 10, 2017

@callodacity We've discussed this before, I don't think replicating fs errors is a good practice.

  1. It's impossible to fully replicate a true native error.
  2. If the error is coming from fs-extra, that should be clear, since fs-extra might have a bug.

@ghost
Copy link

ghost commented Aug 10, 2017

@RyanZim both good points, and I agree with you completely.

However, the only thing I see as an issue is that in order to check if the error thrown is the already exists error, one would have to search the string. I just thought it may be simpler is an error code or number were added (so users could just check for that rather than searching the string for already exists.

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

Successfully merging this pull request may close these issues.

None yet

3 participants