-
Notifications
You must be signed in to change notification settings - Fork 21
Added option for Gulp logging #9
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
Conversation
index.js
Outdated
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.
Couldn't this just be if (options.log) {
?
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.
Generally yes but for options I'm making it specific to avoid people getting true when entering 1, !0, !null, etc. Check this out for some interesting reading: http://dorey.github.io/JavaScript-Equality-Table/
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.
!0
and !null
are === true
. Sure 1
isn't === true
but why restrict it? Let's allow them to pass any "truthy" value to turn on logging.
The README would need to change. E.g. lines like this:
Maybe we should also say it defaults to |
Do you know if this is testable? |
Not sure if it's unit testable but I have tested it manually. It's a fairly simple piece of code. |
Yeah. Thanks. |
Added option for Gulp logging
Made a small change (8d39e6f) and published as |
Yep awesome - no point building the message if we're not logging it. Thanks mate. |
Nothing is foolproof. I managed to break it in 3.0.0. Just added tests, fixed it and published now as 3.0.1. |
Typically Gulp plugins suppress the output of the native module and add an option to log the output. Basically:
Gives the option of keeping the command line clean.