Skip to content
New issue

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

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

Already on GitHub? # 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
@RyanZim RyanZim requested a review from jprichardson December 24, 2016 18:55
@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.

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

Successfully merging this pull request may close these issues.

3 participants