Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

CLI: switch successful message event type #841

Closed
wants to merge 3 commits into from
Closed

CLI: switch successful message event type #841

wants to merge 3 commits into from

Conversation

gibatronic
Copy link

hey guys. this pull request is meant to solve a simple issue:
send CLI succesful messages to stdout instead of stderr.

currently redirecting the stdout has no effect:

node-sass input.scss output.css > /dev/null

the above command still outputs:

Rendering Complete, saving .css file...
Wrote CSS to path/to/output.css

I could redirect the stderr, but then it would be impossible to know if something breaks.

what do you guys think?

@am11
Copy link
Contributor

am11 commented Apr 7, 2015

LGTM!

Please rebase with master: git remote add node-sass https://github.com/sass/node-sass; git pull --rebase node-sass master. I have just merged #838 (there might be other instances you want to fix).

this way it will be correctly sent to stdout instead of stderr
@gibatronic
Copy link
Author

done! there weren't any other instances to update.

@am11
Copy link
Contributor

am11 commented Apr 7, 2015

Thanks!

Going by this logic, we should probably replace the following instances as well:

Meaning we don't need emitter to warn at all? So lets remove:

as well and fix the related tests: probably somewhere bin.stderr.once would need to be replaced by bin.stdout.once in test/cli.js..

@wesleytodd, @paulcpederson, @kevva, thoughts?

@gibatronic
Copy link
Author

uhmm... right!

@paulcpederson
Copy link
Contributor

Using warn for a success message did feel a bit odd. Great idea!

@gibatronic
Copy link
Author

sorry, I don't understand why should watch the full sass dep tree for a single file has failed.

@gibatronic
Copy link
Author

oh I see... with Node v0.12.2 and io.js v1.6.4 everything is fine, the issue is with Node v0.10.38

@gibatronic
Copy link
Author

nope. even with v0.10.38 everything went fine locally...

@wesleytodd
Copy link
Contributor

Ok, so after digging in here I see the problem. This PR breaks standard unix programing pattern of stdout being for the IO of the actual program. So the failing test are because you actually broke that feature.

This is actually why the logging is all supposed to go to stderr. Most likely this PR needs to be denied. The only way this might be acceptable is behind a flag. Or maybe adding a --silent flag so you can suppress the logging to stderr if you have something that requires that.

For more reference: http://en.wikipedia.org/wiki/Standard_streams

Standard output is the stream where a program writes its output data.

Standard error is another output stream typically used by programs to output error messages or diagnostics.

@wesleytodd
Copy link
Contributor

@gibatronic Maybe you should post more about why you need this feature? Then we could brainstorm something better to help?

@gibatronic
Copy link
Author

it's alright, it's probably not valuable enough for common usage.
I like the idea of a silent flag, but only to suppress warn events.
my need is to only show error events, the warning ones I would like to hid.
does it make sense for you guys?

@am11
Copy link
Contributor

am11 commented Apr 8, 2015

Agree with @wesleytodd; this kind of output should not appear on stdout. So lets keep the console.warn and provide the said boolean flag.
As for the flag name, how about --silent-warnings or --mute-warnings?

@paulcpederson
Copy link
Contributor

@wesleytodd +1 good sleuthing.

Having a flag to suppress console.warn messages would actually be really useful. Especially for people who are compiling lots of files.

As for the name, I've seen --quiet used in a couple projects like live-server, supervisor,and passed to git rm (and probably other git commands). Not sure if that is descriptive/obvious enough, though...

@am11
Copy link
Contributor

am11 commented Apr 8, 2015

As for the name, I've seen --quiet

Note that we need to suppress warnings only and keep the errors on stderr.

@wesleytodd
Copy link
Contributor

--quiet fits better when we are not suppressing all output, IMHO.

@gibatronic
Copy link
Author

yea... --silent-warnings or --mute-warnings sounds great!

@paulcpederson
Copy link
Contributor

Good point. 👍

@gibatronic
Copy link
Author

alright! I'll work on the --silent-warnings flag then!
thanks for your time guys, excellent thoughts.
soon I'll be making another pull request then.

@gibatronic gibatronic closed this Apr 8, 2015
@gibatronic gibatronic deleted the cli-switch-successful-message-event-type branch April 8, 2015 16:21
@wesleytodd
Copy link
Contributor

Personally I would choose --quite because, as @paulcpederson mentioned, it is a commonly used flag in other projects. --silent-warnings is a flag I have not seen used before, and although it makes perfect sense, I think there are benefits to following the herd in this.

@paulcpederson
Copy link
Contributor

A good example is git rm --quiet. Git still keeps errors on stderr, it just stops writing warnings like rm 'thing' when it removes things. That's what we're talking about here, right? Completely possible I don't understand the desired behavior correctly...

@gibatronic
Copy link
Author

not a problem, I'm fine with --quiet. I'm really up to you guys.
searching for it in Github I got this numbers:

  1. --quiet1,191,999
  2. --silent552,757
  3. --mute3,636

@am11
Copy link
Contributor

am11 commented Apr 8, 2015

--quite (with unreserved alias -q) is fine and niftier than --mute-warnings, if it delivers the meanings. My suggestion was based on the verbosity.

Hey, I am spoiled by windows powershell: -ErrorAction SilentlyContinue! 😸

@xzyfer xzyfer mentioned this pull request Apr 20, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants