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

Add a limit option to mkdirs() #22

Closed
wants to merge 2 commits into from
Closed

Conversation

jric
Copy link

@jric jric commented Mar 29, 2013

This is a backward-compatible extension that pulls in no extra dependencies. It makes use of mkdirs() safer, by allowing the developer to specify how many levels of hierarchy are allowed to be created by mkdirs(). Because mkdirs() will happily create whatever you tell it to, sometimes you don't find the error in your logic right away and you end up creating directories in unintended places.

Another benefit of this extension is that it gives you the semantics of "ensureDirectory()" from some other fs extensions. The idea behind this is that you want to create a directory if it doesn't already exist, but you don't want an error if it does already exist. In this case, you don't want a hierarchy of directories created, just a single directory, and it would be a legitimate error if the parent directory did not exist. You can do this with the base fs functions, but it gets a little ugly and requires seven lines of code where you only need one.

@jprichardson
Copy link
Owner

Thanks for your contribution! I'll review this on Monday.

@jprichardson
Copy link
Owner

Hi Joshua,

OK, I'm not sure that I see the value other than added complexity of adding hierarchy as a parameter. Can you explain a clear practical use case? Other than that, I agree on the ensureDirectory() style semantics, that's a good idea.

Thanks again.

@jric
Copy link
Author

jric commented Apr 6, 2013

A possible use-case is that you grab a directory from a config file. You expect that directory to exist, but you want to create a subtree there. If the root directory does not exist, it means that there is likely a misconfiguration, and you don't want to be creating new directory structures in the wrong place! So,

var sysdir = config['SYSDIR'], daemon_dir = path.join(sysdir, 'log', 'daemon'),
    util_dir = path.join(sysdir, 'log', 'util');
fs.mkdirs(logdir, 1, callback); // create dir and one parent if necessary
fs.mkdirs(vardir, 1, callback);

is a shortcut for

var sysdir = config['SYSDIR'], daemon_dir = path.join(sysdir, 'log', 'daemon'),
    util_dir = path.join(sysdir, 'log', 'util');
var mkSubdir = function (subdir, callback) {
    fs.exists(sysdir, function (err) {
        if (err) { return callback(err); }
        fs.mkdirs(subdir, callback);
    });
};
mkSubdir(logdir, callback);
mkSubdir(vardir, callback);

@jprichardson
Copy link
Owner

I've gone back and forth on this one and have reviewed this multiple times; I see the utility. It's such an edge case though, that I'm going to have to reject it. Sorry. It seems that this behavior should be at the app level.

I still think that the ensureDirectory style semantics are a great idea though.

Thanks for the contribution and interest.

@jprichardson jprichardson mentioned this pull request Oct 8, 2013
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.

2 participants