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

Handle Errors Gracefully #78

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

coolaj86
Copy link

@coolaj86 coolaj86 commented Aug 13, 2018

Addresses #76

Skip over ENOENT files, return bogus stat object for other fs.stat errors.

mtime: new Date(0),
error: err.toString(),
code: err.code,
isDirectory: function () { return false }
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe your changes will expose this stat object to the users in the previous commits. I assume you made these fake values to prevent code from breaking, but it'll still break since you didn't provide all the values in a native Node.js stat object here. You'll want to make sure all the properties are covered if you're going to provide them at all, otherwise that's going to be even more confusing to the user, imo.

Copy link
Author

Choose a reason for hiding this comment

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

I just pushed another commit to the stat-first branch that converts to a w3 FileAPI-esque object sooner, which corrects that issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will that show up in this pr at all?

Copy link
Author

Choose a reason for hiding this comment

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

No, I made separate branches for each change and only based each change on the fewest number of commits needed to demonstrate the change.

All of the PRs are in order. I’ll go back and add when other PR the depend on.

var fileList = files.map(function (file, i) {
return { name: file, stat: stats[i] };
});
}).filter(function (file) { return file.stat });
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this actually be filtering anything out since the code below is providing a stat object even on error?

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

Successfully merging this pull request may close these issues.

None yet

2 participants