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

Fixes #319: Instructs --output to create given directory recursively. #320

Merged
merged 3 commits into from Mar 4, 2015

Conversation

@ghost
Copy link

ghost commented Mar 3, 2015

This fixes the issue.

@lloydbenson

This comment has been minimized.

Copy link
Contributor

lloydbenson commented Mar 3, 2015

I can't comment if the maintainer will want this patch, but I know that the Mkdir package is problematic. There is a policy of no 0.x packages. I'd try just strictly using fs.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 3, 2015

Oh I didn't know there was such a restriction. Especially since both source-map-support and eslint are 0.x packages. Anyways, if that's the case I'll find some time on the weekend and fix this in a different way.

var dest = typeof options.output === 'string' ? Fs.createWriteStream(options.output) : options.output;
var dest;
if (typeof options.output === 'string') {
Mkdirp.sync(Path.dirname(options.output));

This comment has been minimized.

Copy link
@geek

geek Mar 3, 2015

Member

we are 4 space indenters

@geek geek added the feature label Mar 3, 2015
@geek geek added this to the 5.4.0 milestone Mar 3, 2015
@geek geek self-assigned this Mar 3, 2015
@geek

This comment has been minimized.

Copy link
Member

geek commented Mar 3, 2015

Please add a test for this new functionality

@geek

This comment has been minimized.

Copy link
Member

geek commented Mar 3, 2015

@jzorn and @lloydbenson we try to stick with v1 modules when they are relied on by hapi in production. Since lab is used for development it's sort of the exception on what it depends on. Of course, it would be nice if a module that's ready for prime time is at least v1.

@lloydbenson

This comment has been minimized.

Copy link
Contributor

lloydbenson commented Mar 3, 2015

That's right my apologies. Still seems like you could just use fs and not have the dependency but that's @geek 's call =) Sorry for the diversion!

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 3, 2015

Sweet, sounds great - I'll fix the code style and add some tests and update the PR.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Mar 4, 2015

Imo if you can do someting native without a dependency don't add another dependency

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Mar 4, 2015

@AdriVanHoudt that goes against the very principle of npm, small modules that do few things but do it well.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Mar 4, 2015

@Marsup I agree on that but if the module can be replaced by 5 lines of code it seems like more overhead. I looked at the code and it can't be done in 5 lines so lgtm :D
There does seem to be an issue with infinite loops on windows...

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 4, 2015

Apparently recursively creating folders is not as easy as it appears. I strongly prefer code that is literally used in hundreds of projects over writing my own solutions, trying to cover every pitfall and corner case.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Mar 4, 2015

@jzorn you're right, I thought it would be easier but it turns out not to be so using the module here is prefered

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 4, 2015

Great - I fixed the indent and will write a test and then check back with you.

if (typeof options.output === 'string') {
Mkdirp.sync(Path.dirname(options.output));
dest = Fs.createWriteStream(options.output);
} else {

This comment has been minimized.

Copy link
@Marsup

Marsup Mar 4, 2015

Member

line break before else

geek added a commit that referenced this pull request Mar 4, 2015
Fixes #319: Instructs --output to create given directory recursively.
@geek geek merged commit 0bf3fd3 into hapijs:master Mar 4, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.