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

isDirectory method #132

Closed
wants to merge 1 commit into from

Conversation

overlookmotel
Copy link
Contributor

This PR adds convenience methods isDirectory() and isDirectorySync().

They are shortcuts for fs.stat() followed by calling .isDirectory() on the result. These methods are in my fs-extra-promise module, but you said previously that you thought this could be useful to add to fs-extra, and it seems more natural to me to have them here. So here they are!

I've called them isDirectory to match the native node method name, but also aliased them as isDir to fit with the naming of other methods in fs-extra.

I didn't write any tests as (a) I'm not sure how to exactly within your testing framework and (b) the tests would basically be comprised of the same code as the methods themselves!

Please let me know if you'd like any amends to this PR.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.47%) to 88.4% when pulling 2e3f61f on overlookmotel:isDirectory into 2e9ac77 on jprichardson:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.47%) to 88.4% when pulling a8cdd4a on overlookmotel:isDirectory into 2e9ac77 on jprichardson:master.

@jprichardson
Copy link
Owner

I can see the case for this, but I'm trying to keep this library as minimalistic as possible. I'm inclined to reject this. I'm going to leave it open to think it over for awhile. Thanks for your interest.

@overlookmotel
Copy link
Contributor Author

OK, no problem. I just made the PR as in previous conversation (#101 (comment)) you said you liked it and would probably add it. So I thought I'd save you the trouble!

But I can see the argument in favour of minimalism, so it's totally up to you...

@overlookmotel
Copy link
Contributor Author

By the way, having looked through the way tests are constructed while doing the EEXISTS fix, if you do want to merge this, I can write some tests for it. Please let me know if you want this.

@overlookmotel
Copy link
Contributor Author

Just wondered if you'd had any further thoughts about whether you might like to include this functionality in fs-extra, or not?

@jprichardson
Copy link
Owner

I have thought about it. I'm still undecided. But it's probably better to just make a decision now and move forward and revisit it later. So for now, I'll reject it. If in the future I change my mind, I'll ping you. Thanks for taking the time and the interest though :)

@overlookmotel
Copy link
Contributor Author

OK, no worries. I can understand your reasoning.

}

function isDirectorySync (path) {
return fs.statSync(path).isDirectory()
Copy link

@yvele yvele Nov 30, 2017

Choose a reason for hiding this comment

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

Isn't isDirectorySync supposed not to throw if the path doesn't exists? Because it will throw if you don't catch statSync for ENOENT errors.

The strange thing is that isDirectory swallows the errors and isDirectorySync not.. they don't have the same behavior. Edit: Oops.. I didn't noticed the if (err) return callback(err)

@overlookmotel I think you have the exact same problem here: https://github.com/overlookmotel/fs-extra-promise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yvele I don't see it as a bug. I think it should throw if the path doesn't exist, same as calling fs.stat()/fs.statSync() would.

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

Successfully merging this pull request may close these issues.

4 participants