CLI: Adds --quiet flag

[paulcpederson]

Don't want to step on toes here if you were already working on this, @gibatronic , but thought I'd take a stab at implementing the -q, --quiet flag for the cli.

I think this addresses the use case of #841

Wrote some tests to ensure that the flag keeps the cli from outputting to stderr on log and warn. Also wrote a test to ensure that error is still passed through, regardless of if --quiet is used by creating an invalid fixture which contains a reference to an undefined sass variable.

@gibatronic would this successfully enable you to do what you need to do?
/cc @am11 @wesleytodd

[wesleytodd]

The failure doesn't seem to have anything to do with the PR. So, looks perfect to me!!

[paulcpederson]

@wesleytodd yeah, was just investigating that. Everything seems to pass locally, and the test that it is failing on is testing .render which is the same.

[paulcpederson]

Ok, I think this is because master is failing the build as well.

[gibatronic]

yes @paulcpederson, this is going to work fine.
I was just about to start working on it, so it's alright!

[paulcpederson]

I think #854 addresses this. Should probably wait to merge until that pr is in.

[paulcpederson]

@gibatronic 👍

[saper]

Cool, I think that node-sass should be quiet by default. What messages are we getting on a typical use case?

I think that warnings should be on by default, normal log should be off unless --verbose or -v is given.

Now:

[ ]

node-sass/bin/node-sass

Line 293 in 3d44ce8

emitter.emit('warn', util.format('=> changed: %s', file));

(warn on file change - should be changed to log and quiet by default)
[ ]

node-sass/bin/node-sass

Line 318 in 3d44ce8

emitter.emit('warn', util.format('Wrote %s CSS files to %s', arr.lenth, outputDir));

emitter.emit('warn', util.format('Wrote %s CSS files to %s', arr.lenth, outputDir)); should be log and quiet

Mainly this is about whether watcher should be quiet or not. Anything else that gets generated?

[saper]

I think #856 proves nobody needs those messages :)

[wesleytodd]

Glad I took a second look at this, I think we fell prey to the stdout is for output thing again. If you passed --quite while using this you will get no output when using this in the unix fashion.

Render uses log to ouput when we don't have a dest file: https://github.com/sass/node-sass/blob/master/lib/render.js#L54

So you probably need to remove the one on log.

[wesleytodd]

@saper For the --verbose thing, that would be a breaking change and would require a major version bump. Personally I don't think it is necessary to make that change. I think we should stick with --quite because it is helpful to know that something happened most of the time.

Also this is consistent other tools like gulp and browserify which output helpful confirmation messages when they do something.

[saper]

Those sould simply go away in my opinion... https://github.com/sass/node-sass/blob/master/lib/render.js#L54

Re breaking change: we are still not at level 3.0.0, it's a good time to clean up logging!
As much as I would love to clean up stacktraces and debug messages, but that's a bit another story.

When redesigning logging we should keep also #646 in mind

We have a bit of a design choice: Do we prefer chatty npm/http-server style or quiet UNIX? :)

I think chatty is okay when doing --watch - otherwise it should be pretty quiet...

[wesleytodd]

Maybe we are not on the same page here, those lines enable you to pipe the output in a unix fashion. So removing them would break this use case: $ node-sass file.scss | some-linting-program

The emit log there is actually the quite UNIX method you mentioned above.

[saper]

Ok, let me actually try how it works...

[wesleytodd]

Seems like the warns are what you want to get rid of by default. right @saper?

[saper]

No, I think warns should be there, but some messages we have here don't qualify as a "warning". Maybe there is a problem with the default logging hierarchy.

[wesleytodd]

The idea as I understand it now is this:

• console.log (stdout): Actual program output, so the compiled sass
• console.warn (stderr): Helpful debug messages, like how many files were compiled
• console.error (stderr): Hard errors, probably stop execution

So the idea would be to make the console.warn lines off by default. Then enable them with the --verbose flag. Do I have that right?

[saper]

Need to think about it a bit... node sends .log/.info to the stdout by default, which is "strange by UNIX standards". I need to think a little about what we have and try some things, will come back :)

Would be great to document all places where we log something. stdout should really be reserved for the output (like node-sass < input.scss > output.css)....

[paulcpederson]

@wesleytodd nice catch on render emitting on log. Good 👀 ! I'll fix that and add a test for it.

As per the grander conversation of quiet vs. verbose really just two ways of looking at the same thing. I've seen it both ways and I think both are valid. My 2¢:

1. reserve stdout for actual output at all times
2. Use stderr for warnings and verbose logging
3. Provide a way to turn off that logging (either by default, or via the quiet flag)

Right?

[paulcpederson]

@wesleytodd ok, updated it so that --quiet only removes warn messages. Also added a test to node-sass < in.scss that checks to make sure that everything still renders when you use --quiet

I think toggling off warn messages is what we really want this flag to do anyways. We don't ever call log from the cli itself, we only use warn for relaying information.

Totally willing to flip this pr so that it is a --verbose flag if that is where we want to go.

[wesleytodd]

Personally I like how it works now. But I can see the argument for it being more unix like with no output. Maybe getting more core team members opinions would be good before making a decision one way or another?

@akhleung @am11@andrew @deanmao @kevva @nschonni Sorry if I bothered anyone who is not involved anymore, I just tagged the top ppl on the contributors list.

[am11]

LGTM.

@xzyfer, IMO @wesleytodd and @paulcpederson have better insights on CLI aspect.

[xzyfer]

LGTM.

IMO the average needs/wants a certain amount of output to know that something has happened. I'd be hesitant to go too quiet by default.

If I've followed the conversation correctly, I also agree that we current use warn incorrectly in many places where info would be better suited - that's a conversation for another PR.

[saper]

Yes, unfortunately "warn" seems to be the lowest "stderr" level by default (maybe the emitter could change this). Also --watch makes things really different (this should be kind of verbose by default).

[xzyfer]

[--watch] should be kind of verbose by default

Agreed. I don't believe this PR changes that behaviour unless the --quiet flag is also passed.

[paulcpederson]

Yep. This pr leaves the current behavior as is, just adds the ability to turn that off.

After #854 gets merged I'll rebase so that the tests pass.

[xzyfer]

@paulcpederson it is done!

[paulcpederson]

@xzyfer nice! passing tests! Nice work @saper 👍

[xzyfer]

😭 🎉

[xzyfer]

Merging this in preparation for the next beta release.

Great work @paulcpederson! Thanks for all your feedback @saper

[gibatronic]

👍

[paulcpederson]

🎉