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

Use glob and fs instead of Grunt, fixes #2 #7

Merged
merged 1 commit into from
Jun 25, 2014
Merged

Use glob and fs instead of Grunt, fixes #2 #7

merged 1 commit into from
Jun 25, 2014

Conversation

markdalgleish
Copy link
Collaborator

No description provided.

@markdalgleish
Copy link
Collaborator Author

EDIT: I've fixed the broken Travis build for Node v0.8 by updating npm before installing dependencies.

@tschaub
Copy link
Owner

tschaub commented Jun 24, 2014

I've fixed the broken Travis build for Node v0.8 by updating npm before installing dependencies.

Nice. Was going to suggest the same. Unfortunate to have to do this, but until a new 0.8 is out, everybody will have to patch it.

@@ -70,23 +71,24 @@ exports.publish = function publish(config, done) {
// override defaults with any task options
var options = _.extend({}, defaults, config);

if (!grunt.file.isDir(options.base)) {
if (!fs.statSync(options.base).isDirectory()) {
Copy link
Owner

Choose a reason for hiding this comment

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

Since statSync will throw if options.base is undefined or it doesn't exist, this would have to be wrapped in try/catch to maintain previous behavior.

@tschaub
Copy link
Owner

tschaub commented Jun 24, 2014

Looks good @markdalgleish. One minor comment on error handling with a bad options.base. Would make for nicer messages for the user if we caught cases where options.base is undefined or a path to a file that doesn't exist.

@markdalgleish
Copy link
Collaborator Author

I've wrapped the fs.statSync call in a try/catch. I don't think we need to do more than that, since the error messages from fs.statSync seem pretty clear to me.

Since base is required, and defaulting to process.cwd() without any prompt to the user is a massive footgun, should we change the API to the following?

ghpages.publish(basePath, options, callback);

@tschaub
Copy link
Owner

tschaub commented Jun 25, 2014

One change this introduces is that the src config could previously be an array and could include negated patterns. But I think we can merge this and add that functionality back in separately.

ghpages.publish(basePath, options, callback);

That sounds good to me (also can be done separately).

tschaub added a commit that referenced this pull request Jun 25, 2014
Use glob and fs instead of Grunt, fixes #2.
@tschaub tschaub merged commit f39a37e into tschaub:master Jun 25, 2014
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