From 4af2de0f095cc7a994d65e5d36dda60b7af25c82 Mon Sep 17 00:00:00 2001 From: Vasili Novikov Date: Fri, 24 May 2019 21:46:22 +0200 Subject: [PATCH] respect tty discovery for CLICOLOR specs This commit also: * fixes the code to prioritise CLICOLOR_FORCE over NO_COLOR. The docs said that there is this preference, but in fact the opposite preference took place before. (Pattern match had wrong order of lines.) * removes the prioritization example table because it was wrong, and it may be harder to read and maintain than the text description above. (It was wrong on line 5.) * fix a specs mismatch in handling `CLICOLOR_FORCE`. A value of `0` should be treated identically to unset. * make `ShouldColorize.clicolor` be `bool` instead of `Option`, because the optional boolean does not hold any meaning really, we always practically unwrap it as `.unwrap_or_else(|| true)`. * fix and add tests in accordance to the specs fixes GH-54 --- Cargo.toml | 1 + src/control.rs | 91 +++++++++++++++++++++++--------------------------- src/lib.rs | 4 ++- 3 files changed, 46 insertions(+), 50 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 17220b8..d54fc46 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,6 +14,7 @@ keywords = ["color", "string", "term", "ansi_term", "term-painter"] no-color = [] [dependencies] +atty = "0.2.11" lazy_static = "1.2.0" [target.'cfg(windows)'.dependencies] diff --git a/src/control.rs b/src/control.rs index ac38409..bd6537c 100644 --- a/src/control.rs +++ b/src/control.rs @@ -23,14 +23,14 @@ use winconsole::{console, errors::WinResult}; /// ``` #[cfg(windows)] pub fn set_virtual_terminal(use_virtual: bool) -> WinResult<()> { - let mut mode = console::get_output_mode()?; - mode.VirtualTerminalProcessing = use_virtual; - console::set_output_mode(mode)?; - Ok(()) + let mut mode = console::get_output_mode()?; + mode.VirtualTerminalProcessing = use_virtual; + console::set_output_mode(mode)?; + Ok(()) } pub struct ShouldColorize { - clicolor: Option, + clicolor: bool, clicolor_force: Option, // XXX we can't use Option because we can't use &mut references to ShouldColorize has_manual_override: AtomicBool, @@ -56,7 +56,7 @@ lazy_static! { impl Default for ShouldColorize { fn default() -> ShouldColorize { ShouldColorize { - clicolor: None, + clicolor: true, clicolor_force: None, has_manual_override: AtomicBool::new(false), manual_override: AtomicBool::new(false), @@ -65,25 +65,16 @@ impl Default for ShouldColorize { } impl ShouldColorize { - /// Reads environment variables to determine whether colorization should - /// be used or not. `CLICOLOR_FORCE` takes highest priority, followed by - /// `NO_COLOR`, followed by `CLICOLOR`. In the absence of manual overrides, - /// which take precedence over all environment variables, the priority - /// of these variables can be expressed as follows. - /// - /// `NO_COLOR` | `CLICOLOR` | `CLICOLOR_FORCE` | colorize? - /// :--------- | :--------- | :--------------- | :-------- - /// unset | unset | unset | true (default) - /// unset | `!= 0` | unset | true - /// unset | `== 0` | unset | false - /// set | unset/`== 0`/`!= 0` | unset | false - /// set/unset | unset/`== 0`/`!= 0` | `== 0` | false - /// set/unset | unset/`== 0`/`!= 0` | `!= 0` | true + /// Reads environment variables and checks if output is a tty to determine + /// whether colorization should be used or not. + /// `CLICOLOR_FORCE` takes highest priority, followed by `NO_COLOR`, + /// followed by `CLICOLOR` combined with tty check. pub fn from_env() -> Self { use std::io; ShouldColorize { - clicolor: ShouldColorize::normalize_env(env::var("CLICOLOR")), + clicolor: ShouldColorize::normalize_env(env::var("CLICOLOR")).unwrap_or_else(|| true) + && atty::is(atty::Stream::Stdout), clicolor_force: ShouldColorize::resolve_clicolor_force( env::var("NO_COLOR"), env::var("CLICOLOR_FORCE"), @@ -101,11 +92,7 @@ impl ShouldColorize { return forced_value; } - if let Some(value) = self.clicolor { - return value; - } - - true + self.clicolor } pub fn set_override(&self, override_colorize: bool) { @@ -131,13 +118,12 @@ impl ShouldColorize { no_color: Result, clicolor_force: Result, ) -> Option { - match ( - ShouldColorize::normalize_env(no_color), - ShouldColorize::normalize_env(clicolor_force), - ) { - (_, Some(b)) => Some(b), - (Some(_), None) => Some(false), - (None, None) => None, + if ShouldColorize::normalize_env(clicolor_force) == Some(true) { + Some(true) + } else if ShouldColorize::normalize_env(no_color).is_some() { + Some(false) + } else { + None } } } @@ -179,7 +165,7 @@ mod specs { ctx.describe("::resolve_clicolor_force", |ctx| { ctx.it( - "should return None if neither NO_COLOR nor CLICOLOR_FORCE are set", + "should return None if NO_COLOR is not set and CLICOLOR_FORCE is not set or set to 0", || { assert_eq!( None, @@ -188,11 +174,18 @@ mod specs { Err(env::VarError::NotPresent) ) ); + assert_eq!( + None, + ShouldColorize::resolve_clicolor_force( + Err(env::VarError::NotPresent), + Ok(String::from("0")), + ) + ); }, ); ctx.it( - "should return Some(false) if NO_COLOR is set and CLICOLOR_FORCE is unset", + "should return Some(false) if NO_COLOR is set and CLICOLOR_FORCE is not enabled", || { assert_eq!( Some(false), @@ -208,11 +201,18 @@ mod specs { Err(env::VarError::NotPresent) ) ); + assert_eq!( + Some(false), + ShouldColorize::resolve_clicolor_force( + Ok(String::from("1")), + Ok(String::from("0")), + ) + ); }, ); ctx.it( - "should ignore NO_COLOR and return Some(forced_value) if CLICOLOR_FORCE is set to forced_value", + "should prioritize CLICOLOR_FORCE over NO_COLOR if CLICOLOR_FORCE is set to non-zero value", || { assert_eq!( Some(true), @@ -228,13 +228,6 @@ mod specs { Ok(String::from("0")), ) ); - assert_eq!( - Some(false), - ShouldColorize::resolve_clicolor_force( - Err(env::VarError::NotPresent), - Ok(String::from("0")), - ) - ); assert_eq!( Some(true), ShouldColorize::resolve_clicolor_force( @@ -259,7 +252,7 @@ mod specs { ctx.describe("when only changing clicolors", |ctx| { ctx.it("clicolor == false means no colors", || { let colorize_control = ShouldColorize { - clicolor: Some(false), + clicolor: false, ..ShouldColorize::default() }; false == colorize_control.should_colorize() @@ -267,7 +260,7 @@ mod specs { ctx.it("clicolor == true means colors !", || { let colorize_control = ShouldColorize { - clicolor: Some(true), + clicolor: true, ..ShouldColorize::default() }; true == colorize_control.should_colorize() @@ -283,7 +276,7 @@ mod specs { "clicolor_force should force to true no matter clicolor", || { let colorize_control = ShouldColorize { - clicolor: Some(false), + clicolor: false, clicolor_force: Some(true), ..ShouldColorize::default() }; @@ -296,7 +289,7 @@ mod specs { "clicolor_force should force to false no matter clicolor", || { let colorize_control = ShouldColorize { - clicolor: Some(true), + clicolor: true, clicolor_force: Some(false), ..ShouldColorize::default() }; @@ -309,7 +302,7 @@ mod specs { ctx.describe("using a manual override", |ctx| { ctx.it("shoud colorize if manual_override is true, but clicolor is false and clicolor_force also false", || { let colorize_control = ShouldColorize { - clicolor: Some(false), + clicolor: false, clicolor_force: None, has_manual_override: AtomicBool::new(true), manual_override: AtomicBool::new(true), @@ -321,7 +314,7 @@ mod specs { ctx.it("should not colorize if manual_override is false, but clicolor is true or clicolor_force is true", || { let colorize_control = ShouldColorize { - clicolor: Some(true), + clicolor: true, clicolor_force: Some(true), has_manual_override: AtomicBool::new(true), manual_override: AtomicBool::new(false), diff --git a/src/lib.rs b/src/lib.rs index 7a50d85..8d77a07 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -24,6 +24,7 @@ //! See [the `Colorize` trait](./trait.Colorize.html) for all the methods. //! +extern crate atty; #[macro_use] extern crate lazy_static; #[cfg(windows)] @@ -173,7 +174,8 @@ impl ColoredString { // TODO: BoyScoutRule let reset = "\x1B[0m"; let style = self.compute_style(); - let matches: Vec = self.input + let matches: Vec = self + .input .match_indices(reset) .map(|(idx, _)| idx) .collect();