-
Notifications
You must be signed in to change notification settings - Fork 17
Conversation
Looks good to me, I have a single question. Why do you need the noInstall option? Does the test fail on Travis if you try to install the dependencies? Or did you add it because otherwise the test takes too much time? |
@@ -131,7 +154,8 @@ module.exports = function(config) { | |||
.pipe(template(templateConfig)) | |||
.pipe(conflict(rootDir)) | |||
.pipe(gulp.dest(rootDir)) | |||
.pipe(install()); | |||
.pipe(config.noInstall ? util.noop() : install()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: couldn't you directly use sink here instead of util.noop and then sink?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Install also doesn't work if you don't do a sink().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok. Could you update the comment in the following line then?
Mainly because it's slow as mentioned above. Let me try on travis and see if how it's doing. |
Oops, I've skimmed over the first comment! I really didn't notice you wrote something in it 😄 |
There are a few npm ERR! tar.unpack untar error and then the test times out. |
a0d1405
to
e73e234
Compare
Add basic test for bootstrap.
I had some issues with the gulp-install not triggering a finish event, but that can be fixed with calling sink() at the end. However, I have install disabled for tests as it is slow and require network connectivity.
Fixes #36