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

Emit plugin error on failure #6

Merged
merged 1 commit into from
Jul 14, 2014
Merged

Emit plugin error on failure #6

merged 1 commit into from
Jul 14, 2014

Conversation

eduardolundgren
Copy link
Contributor

When not able to parse it should emit an error to be able to catch from the stream level.

When not able to parse it should emit an error to be able to catch from the stream level.
koistya added a commit that referenced this pull request Jul 14, 2014
Emit plugin error on failure
@koistya koistya merged commit dd7c7e4 into koistya:master Jul 14, 2014
koistya added a commit that referenced this pull request Jul 14, 2014
Emit plugin error on failure
@koistya
Copy link
Owner

koistya commented Jul 14, 2014

Thanks!

@tonyganch
Copy link

processString method accepts file name as third parameter:
https://github.com/csscomb/core#combprocessstringstring-syntax-filename
You could just run var output = comb.processString(file.contents.toString('utf8'), syntax, file.path) without all those buffers and try/catch blocks that slow things down.

@koistya
Copy link
Owner

koistya commented Jul 15, 2014

@tonyganch yeah, I'm already passing file.path to the processString(..) method. But since it throws an exception if parsing fails, I guess wrapping it in a try catch makes sense. No?

https://github.com/koistya/gulp-csscomb/blob/master/index.js#L57-L62

@tonyganch
Copy link

@koistya, I think that for some reason you do a lot of work that has already been done and tested.
Maybe processing files with gulp is really tricky, I don't know.
But for example, processFile(path) checks whether file syntax is supported and uses it.
getCustomConfig() checks whether there is a custom config somewhere and loads it.

A lot of cool stuff is added to CSScomb when people need it.
If you believe that processString should log parser errors instead of throwing them, you can just open an issue/PR and propose/discuss some changes.

@eduardolundgren
Copy link
Contributor Author

Even if processFile(path) checks if the file extension is supported you can still pass a invalid content to it. If it breaks on a stream chain it causes several issues, e.g. stopping a watch listener. Throwing a gulp plugin error is the recommendation and standard for those situations.

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.

3 participants