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

Should not send color to a pipe by default #512

Closed
joshtriplett opened this issue May 23, 2016 · 5 comments
Closed

Should not send color to a pipe by default #512

joshtriplett opened this issue May 23, 2016 · 5 comments
Labels
A-help Area: documentation, including docs.rs, readme, examples, etc... C-enhancement Category: Raise on the bar on expectations
Milestone

Comments

@joshtriplett
Copy link
Contributor

joshtriplett commented May 23, 2016

Clap prints color unconditionally, even when sending output somewhere other than a terminal. Redirecting output to a file, or to a pager that doesn't understand terminal escape sequences like less -R does, produces undesirable terminal escape sequences in the file or pager.

Clap should detect when output goes somewhere other than a terminal, and suppress terminal escape sequences otherwise.

@kbknapp
Copy link
Member

kbknapp commented May 23, 2016

Thanks for filing this. Yeah I agree, and this should be an easy detection on whether not something is a terminal. I'm undecided if this should be something clap handles, or simply makes easy to handle in consumer code. If clap handles it, it's done at compile time. Even though App instances are built at runtime, you'd run into a chicken/egg problem if a user tried passing some imaginary option for to change the functionality from a --color=auto to a --color=never or the like.

I think the best way to accomplish this would be a setting of some sort, perhaps default to ColorAlways, and then allow ColorAuto or ColorNever instead. This leaves it at compile time, but allows for some hacky workarounds if someone does want to support some type of --color=WHEN option (such as env vars, or looping upon conditionally receiving the --color option.)

Thoughts? This should be an easy addition if that option works for you.

@kbknapp kbknapp added C-enhancement Category: Raise on the bar on expectations A-help Area: documentation, including docs.rs, readme, examples, etc... D: easy labels May 23, 2016
@joshtriplett
Copy link
Contributor Author

joshtriplett commented May 23, 2016

I don't think it makes sense to have a command-line option to change this or similar; all of clap's color (or output in general) happens when emitting usage, help, or errors, which happen as part of command-line option processing. A program might want to have an auto/always/never configuration for color, and if it does (perhaps via config file read before the command line) then clap might want to provide a way to feed that in. But I'm suggesting that clap should handle this by default, and default to "auto".

I think it would make sense to detect whether the target terminal (stdout/stderr) is a TTY, and only emit terminal escape sequences if so.

Rust has code to implement this; see https://github.com/rust-lang/rust/blob/master/src/libtest/lib.rs#L809 and https://github.com/rust-lang/rust/blob/master/src/libsyntax/errors/emitter.rs#L476 . This likely needs to move into a library function somewhere; see rust-lang/rust#33736 . Once that library function exists, I'd suggest using it in clap to detect whether stderr goes to a terminal, and only producing escape sequences if so.

Entirely optionally, you could also provide a clap option (for instance, on App) to override this detection and always/never use color.

@kbknapp
Copy link
Member

kbknapp commented May 24, 2016

Sounds good. Since this is for colored output which already isn't implemented on Windows systems I'm not too worried about using libc or not being able to detect this on Windows. libc is already a dep by default, so again, this isn't too huge a deal.

Should have this implemented shortly. 👍

@kbknapp
Copy link
Member

kbknapp commented May 30, 2016

Just an update; I've been on vacation and should have this completed in a few days once I'm home.

@kbknapp
Copy link
Member

kbknapp commented Jun 4, 2016

#520 implements this

@homu homu closed this as completed in 65c2350 Jun 8, 2016
@kbknapp kbknapp added this to the 2.6.0 milestone Jun 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-help Area: documentation, including docs.rs, readme, examples, etc... C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants