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

Added removeCallback passing #25

Merged
merged 2 commits into from
Jul 9, 2014
Merged

Added removeCallback passing #25

merged 2 commits into from
Jul 9, 2014

Conversation

foxel
Copy link
Contributor

@foxel foxel commented Jul 3, 2014

Added removeCallback passing.
Usefull for daemons that need to create some directory and remove it when the work is done.

@@ -167,9 +167,21 @@ function _createTmpFile(options, callback) {
fs.open(name, _c.O_CREAT | _c.O_EXCL | _c.O_RDWR, opts.mode || 0600, function _fileCreated(err, fd) {
if (err) return cb(err);

if (!opts.keep) _removeObjects.unshift([ fs.unlinkSync, name ]);
var removed = false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason the keep the remove variable? Only the removeCallback function could set this var and therefore will be never true at the check.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you're protecting the code from the double call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a protection from double call.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is not enough protection, I think it would be better to do an fs.exists check before the removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just protects from double call.
But not from double delete if user deletes file by themselves.
Also the last variant was not covered earlier. I think it should be a separate change.

@raszi
Copy link
Owner

raszi commented Jul 9, 2014

Please also add tests for this functionality.

@foxel
Copy link
Contributor Author

foxel commented Jul 9, 2014

Added tests. Made some refactoring.

raszi added a commit that referenced this pull request Jul 9, 2014
Added removeCallback passing
@raszi raszi merged commit 7609f95 into raszi:master Jul 9, 2014
@raszi
Copy link
Owner

raszi commented Jul 9, 2014

Great, thank you very much!

@foxel
Copy link
Contributor Author

foxel commented Jul 9, 2014

Cool!
Will you bump up the version?

@raszi
Copy link
Owner

raszi commented Jul 9, 2014

Sure, I'll do that soon.

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.

5 participants