diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cea46bfac2c..adfc0188d811 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,6 +76,19 @@ will become the public version at the next release._ (@aljazerzen, #2605) +- We've changed how we handle colors. We now use the + `[anstream](https://github.com/rust-cli/anstyle)` library in `prqlc` & + `prql-compiler`. + + `Options::color` is deprecated and has no effect. Code which consumes + `prql_compiler::compile` should instead accept the output with colors and use + a library such as `anstream` to handle the presentation of colors. To ensure + minimal disruption, `prql_compiler` will currently strip color codes when a + standard environment variable such as `CLI_COLOR=0` is set or when it detects + `stderr` is not a TTY. + + (@max-sixty, #2773) + **Fixes**: **Documentation**: diff --git a/Cargo.lock b/Cargo.lock index 7d57f8aa0849..6266819ade0e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -792,6 +792,16 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "acbf1af155f9b9ef647e42cdc158db4b64a1b61f743629225fde6f3e0be2a7c7" +[[package]] +name = "colorchoice-clap" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "412e88a3a3a3f52e436909b49beb467a05649e8b0dda0e6202bd05c1b63dbc49" +dependencies = [ + "clap", + "colorchoice", +] + [[package]] name = "combine" version = "4.6.6" @@ -813,46 +823,6 @@ dependencies = [ "unicode-width", ] -[[package]] -name = "concolor" -version = "0.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0b946244a988c390a94667ae0e3958411fa40cc46ea496a929b263d883f5f9c3" -dependencies = [ - "bitflags 1.3.2", - "concolor-override", - "concolor-query", - "is-terminal", -] - -[[package]] -name = "concolor-clap" -version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "435ff0007a3bb04099fe1beedc6b76e7dd5340c90b168008ac0d7e87441de1bf" -dependencies = [ - "clap", - "concolor", -] - -[[package]] -name = "concolor-override" -version = "1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "40a4925288e39d5923e024781971aab940995fa31bab3ffceebbadfc87591e90" -dependencies = [ - "colorchoice", -] - -[[package]] -name = "concolor-query" -version = "0.3.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "88d11d52c3d7ca2e6d0040212be9e4dbbcd78b6447f535b6b561f449427944cf" -dependencies = [ - "windows-sys 0.45.0", -] - [[package]] name = "connection-string" version = "0.2.0" @@ -2155,6 +2125,7 @@ name = "mdbook-prql" version = "0.8.1" dependencies = [ "ansi-to-html", + "anstream", "anyhow", "clap", "globset", @@ -2871,6 +2842,7 @@ dependencies = [ name = "prql-compiler" version = "0.8.1" dependencies = [ + "anstream", "anyhow", "ariadne", "cfg-if", @@ -2953,6 +2925,7 @@ dependencies = [ name = "prqlc" version = "0.8.1" dependencies = [ + "anstream", "anyhow", "ariadne", "atty", @@ -2960,8 +2933,8 @@ dependencies = [ "clap_complete_command", "clio", "color-eyre", - "concolor", - "concolor-clap", + "colorchoice", + "colorchoice-clap", "env_logger", "insta", "insta-cmd", diff --git a/bindings/prql-js/src/lib.rs b/bindings/prql-js/src/lib.rs index eb568eb66e96..5d59421b153c 100644 --- a/bindings/prql-js/src/lib.rs +++ b/bindings/prql-js/src/lib.rs @@ -11,7 +11,7 @@ use wasm_bindgen::prelude::*; pub fn compile(prql_query: &str, options: Option) -> Option { return_or_throw( prql_compiler::compile(prql_query, &options.map(|x| x.into()).unwrap_or_default()) - .map_err(|e| e.composed(&prql_query.into(), false)), + .map_err(|e| e.composed(&prql_query.into())), ) } diff --git a/bindings/prql-lib/src/lib.rs b/bindings/prql-lib/src/lib.rs index 4e530843daa7..c2f96461d95d 100644 --- a/bindings/prql-lib/src/lib.rs +++ b/bindings/prql-lib/src/lib.rs @@ -37,7 +37,7 @@ pub unsafe extern "C" fn compile( .and_then(prql_compiler::pl_to_rq) .and_then(|rq| prql_compiler::rq_to_sql(rq, &opts.unwrap_or_default())) }) - .map_err(|e| e.composed(&prql_query.into(), false)); + .map_err(|e| e.composed(&prql_query.into())); result_into_c_str(result) } diff --git a/bindings/prql-python/src/lib.rs b/bindings/prql-python/src/lib.rs index 6c69e2c8a955..d5e51d20bf67 100644 --- a/bindings/prql-python/src/lib.rs +++ b/bindings/prql-python/src/lib.rs @@ -15,7 +15,7 @@ pub fn compile(prql_query: &str, options: Option) -> PyResult(e.to_string()))) } diff --git a/prql-compiler/Cargo.toml b/prql-compiler/Cargo.toml index 6b28e2943416..9bd5186fa91d 100644 --- a/prql-compiler/Cargo.toml +++ b/prql-compiler/Cargo.toml @@ -11,6 +11,7 @@ version.workspace = true metadata.msrv = "1.65.0" [dependencies] +anstream = {version = "0.3.2", features = ["auto"]} anyhow = {version = "1.0.57", features = ["backtrace"]} ariadne = "0.3.0" csv = "1.2.0" diff --git a/prql-compiler/prqlc/Cargo.toml b/prql-compiler/prqlc/Cargo.toml index 1bc12000010c..303a85a12034 100644 --- a/prql-compiler/prqlc/Cargo.toml +++ b/prql-compiler/prqlc/Cargo.toml @@ -11,6 +11,7 @@ version.workspace = true metadata.msrv = "1.65.0" [target.'cfg(not(target_family="wasm"))'.dependencies] +anstream = {version = "0.3.2", features = ["auto"]} anyhow = {version = "1.0.57"} ariadne = "0.3.0" atty = "0.2.14" @@ -18,8 +19,8 @@ clap = {version = "4.3.0", features = ["derive", "env", "wrap_help"]} clap_complete_command = "0.5.1" clio = {version = "0.2.7", features = ['clap-parse']} color-eyre = "0.6.1" -concolor = "0.1.0" -concolor-clap = {version = "0.1.0", features = ["api"]} +colorchoice = "1.0.0" +colorchoice-clap = "1.0.0" env_logger = {version = "0.10.0", features = ["color"]} itertools = "0.10.3" notify = "^6.0.0" diff --git a/prql-compiler/prqlc/src/cli.rs b/prql-compiler/prqlc/src/cli.rs index 624582fd5b15..1ec0ee3bc5b3 100644 --- a/prql-compiler/prqlc/src/cli.rs +++ b/prql-compiler/prqlc/src/cli.rs @@ -1,3 +1,4 @@ +use anstream::eprintln; use anyhow::bail; use anyhow::Result; use ariadne::Source; @@ -22,7 +23,7 @@ pub fn main() -> color_eyre::eyre::Result<()> { env_logger::builder().format_timestamp(None).init(); color_eyre::install()?; let mut cli = Cli::parse(); - cli.color.apply(); + cli.color.write_global(); if let Err(error) = cli.command.run() { eprintln!("{error}"); @@ -33,12 +34,11 @@ pub fn main() -> color_eyre::eyre::Result<()> { } #[derive(Parser, Debug, Clone)] -#[command(color = concolor_clap::color_choice())] struct Cli { #[command(subcommand)] command: Command, #[command(flatten)] - color: concolor_clap::Color, + color: colorchoice_clap::Color, } #[derive(Subcommand, Debug, Clone)] @@ -170,7 +170,7 @@ impl Command { } }; - output.write_all(pl_to_prql(ast)?.as_bytes())?; + output.write_all(&pl_to_prql(ast)?.into_bytes())?; Ok(()) } Command::ShellCompletion { shell } => { @@ -223,7 +223,7 @@ impl Command { let context = semantic::resolve(stmts, Default::default()) .map_err(prql_compiler::downcast) - .map_err(|e| e.composed(sources, true))?; + .map_err(|e| e.composed(sources))?; let mut out = Vec::new(); for (source_id, source) in &sources.sources { @@ -284,13 +284,12 @@ impl Command { let opts = Options::default() .with_target(Target::from_str(target).map_err(|e| downcast(e.into()))?) - .with_color(concolor::get(concolor::Stream::Stdout).ansi_color()) .with_signature_comment(*hide_signature_comment); prql_to_pl_tree(sources) .and_then(|pl| pl_to_rq_tree(pl, &main_path)) .and_then(|rq| rq_to_sql(rq, &opts)) - .map_err(|e| e.composed(sources, opts.color))? + .map_err(|e| e.composed(sources))? .as_bytes() .to_vec() } @@ -646,15 +645,12 @@ group a_column (take 10 | sort b_column | derive {the_number = rank, last = lag "###); } + /// Check we get an error on a bad input #[test] fn compile() { - // Check we get an error on a bad input // Disable colors (would be better if this were a proper CLI test and // passed in `--color=never`) - concolor_clap::Color { - color: concolor_clap::ColorChoice::Never, - } - .apply(); + anstream::ColorChoice::Never.write_global(); let result = Command::execute( &Command::SQLCompile { @@ -665,7 +661,8 @@ group a_column (take 10 | sort b_column | derive {the_number = rank, last = lag &mut "asdf".into(), "", ); - assert_display_snapshot!(result.unwrap_err(), @r###" + + assert_display_snapshot!(&result.unwrap_err().to_string(), @r###" Error: ╭─[:1:1] │ diff --git a/prql-compiler/src/error.rs b/prql-compiler/src/error.rs index b104bf3d31a1..14ea148b4dd4 100644 --- a/prql-compiler/src/error.rs +++ b/prql-compiler/src/error.rs @@ -1,14 +1,15 @@ +use anstream::adapter::strip_str; pub use anyhow::Result; use ariadne::{Cache, Config, Label, Report, ReportKind, Source}; use serde::de::Visitor; use serde::{Deserialize, Serialize}; -use std::collections::HashMap; use std::error::Error as StdError; use std::fmt::{self, Debug, Display, Formatter}; use std::ops::{Add, Range}; use std::path::PathBuf; +use std::{collections::HashMap, io::stderr}; use crate::SourceTree; @@ -258,7 +259,7 @@ impl ErrorMessages { } /// Computes message location and builds the pretty display. - pub fn composed(mut self, sources: &SourceTree, color: bool) -> Self { + pub fn composed(mut self, sources: &SourceTree) -> Self { let mut cache = FileTreeCache::new(sources); for e in &mut self.inner { @@ -274,20 +275,16 @@ impl ErrorMessages { }; e.location = e.compose_location(source); - e.display = e.compose_display(source_path.clone(), &mut cache, color); + e.display = e.compose_display(source_path.clone(), &mut cache); } self } } impl ErrorMessage { - fn compose_display( - &self, - source_path: PathBuf, - cache: &mut FileTreeCache, - color: bool, - ) -> Option { - let config = Config::default().with_color(color); + fn compose_display(&self, source_path: PathBuf, cache: &mut FileTreeCache) -> Option { + // We always pass color to ariadne as true, and then (currently) strip later. + let config = Config::default().with_color(true); let span = Range::from(self.span?); @@ -312,7 +309,19 @@ impl ErrorMessage { let mut out = Vec::new(); report.finish().write(cache, &mut out).ok()?; - String::from_utf8(out).ok() + + // Strip colors, for external libraries which don't yet strip + // themselves, and for insta snapshot tests. This will respond to + // environment variables such as `CLI_COLOR`. Eventually we can remove + // this, always pass colors back, and the consuming library can strip + // (including insta https://github.com/mitsuhiko/insta/issues/378). + String::from_utf8(out).ok().map(|x| { + if !should_use_color() { + strip_str(&x).to_string() + } else { + x + } + }) } fn compose_location(&self, source: &Source) -> Option { @@ -327,6 +336,15 @@ impl ErrorMessage { } } +fn should_use_color() -> bool { + match anstream::AutoStream::choice(&stderr()) { + anstream::ColorChoice::Auto => true, + anstream::ColorChoice::Always => true, + anstream::ColorChoice::AlwaysAnsi => true, + anstream::ColorChoice::Never => false, + } +} + struct FileTreeCache<'a> { file_tree: &'a SourceTree, cache: HashMap, diff --git a/prql-compiler/src/lib.rs b/prql-compiler/src/lib.rs index 46c700036075..556ce67fc862 100644 --- a/prql-compiler/src/lib.rs +++ b/prql-compiler/src/lib.rs @@ -131,7 +131,7 @@ pub fn compile(prql: &str, options: &Options) -> Result { .and_then(|ast| semantic::resolve_and_lower(ast, &[])) .and_then(|rq| sql::compile(rq, options)) .map_err(error::downcast) - .map_err(|e| e.composed(&prql.into(), options.color)) + .map_err(|e| e.composed(&prql.into())) } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -195,7 +195,15 @@ pub struct Options { /// Defaults to true. pub signature_comment: bool, - /// Whether to use ANSI colors in error messages. + /// Whether to use ANSI colors in error messages. This is deprecated and has + /// no effect. + /// + /// Instead, in order of preference: + /// - Use a library such as `anstream` to encapsulate the presentation + /// logic. + /// - Set an environment variable such as `CLI_COLOR=0` to disable any + /// colors coming back from this library. + /// - Strip colors from the output (possibly also with a library such as `anstream`) pub color: bool, } @@ -231,6 +239,7 @@ impl Options { self } + #[deprecated(note = "`color` now has no effect; see `Options` docs for more details")] pub fn with_color(mut self, color: bool) -> Self { self.color = color; self @@ -248,14 +257,14 @@ pub fn prql_to_pl(prql: &str) -> Result, ErrorMessages> { parser::parse(&sources) .map(|x| x.sources.into_values().next().unwrap()) .map_err(error::downcast) - .map_err(|e| e.composed(&prql.into(), false)) + .map_err(|e| e.composed(&prql.into())) } /// Parse PRQL into a PL AST pub fn prql_to_pl_tree(prql: &SourceTree) -> Result>, ErrorMessages> { parser::parse(prql) .map_err(error::downcast) - .map_err(|e| e.composed(prql, false)) + .map_err(|e| e.composed(prql)) } /// Perform semantic analysis and convert PL to RQ. diff --git a/prql-compiler/src/tests/test.rs b/prql-compiler/src/tests/test.rs index 13e2b0aece52..25df18c8d983 100644 --- a/prql-compiler/src/tests/test.rs +++ b/prql-compiler/src/tests/test.rs @@ -1,9 +1,10 @@ //! Simple tests for "this PRQL creates this SQL" go here. // use super::*; -use crate::{sql, Options, SourceTree, Target}; +use crate::{sql, ErrorMessages, Options, SourceTree, Target}; use insta::{assert_display_snapshot, assert_snapshot}; -pub fn compile(prql: &str) -> Result { +pub fn compile(prql: &str) -> Result { + anstream::ColorChoice::Never.write_global(); crate::compile(prql, &Options::default().no_signature()) } @@ -3532,10 +3533,10 @@ fn test_upper() { } #[test] -fn test_1535() -> anyhow::Result<()> { +fn test_1535() { assert_display_snapshot!(compile(r#" from x.y.z - "#)?, + "#).unwrap(), @r###" SELECT * @@ -3543,8 +3544,6 @@ fn test_1535() -> anyhow::Result<()> { x.y.z "### ); - - Ok(()) } #[test] @@ -3844,3 +3843,14 @@ fn test_type_as_column_name() { foo AS t "###); } + +#[test] +fn test_error_code() { + let err = compile( + r###" + let a = (from x) + "###, + ) + .unwrap_err(); + assert_eq!(err.inner[0].code.as_ref().unwrap(), "E0001"); +} diff --git a/prql-compiler/src/tests/test_error_messages.rs b/prql-compiler/src/tests/test_error_messages.rs index 20edc43026fc..7208133cb519 100644 --- a/prql-compiler/src/tests/test_error_messages.rs +++ b/prql-compiler/src/tests/test_error_messages.rs @@ -90,14 +90,6 @@ fn test_errors() { ───╯ "###); - let err = compile( - r###" - let a = (from x) - "###, - ) - .unwrap_err(); - assert_eq!(err.inner[0].code.as_ref().unwrap(), "E0001"); - assert_display_snapshot!(compile("Answer: T-H-A-T!").unwrap_err(), @r###" Error: ╭─[:1:7] diff --git a/web/book/Cargo.toml b/web/book/Cargo.toml index 148b012d2ec8..21a753d575ce 100644 --- a/web/book/Cargo.toml +++ b/web/book/Cargo.toml @@ -18,6 +18,8 @@ test = false [dependencies] ansi-to-html = "0.1.2" anyhow = "1.0.57" +# Required for one test, can remove when we always pass colors back. +anstream = {version = "0.3.2", features = ["auto"]} globset = "0.4.8" itertools = "0.10.3" prql-compiler = {path = "../../prql-compiler", default-features = false} diff --git a/web/book/src/lib.rs b/web/book/src/lib.rs index 474c24f25e8b..3f86b593150c 100644 --- a/web/book/src/lib.rs +++ b/web/book/src/lib.rs @@ -159,12 +159,7 @@ fn replace_examples(text: &str) -> Result { }; let prql = text.to_string(); - let result = compile( - &prql, - &prql_compiler::Options::default() - .no_signature() - .with_color(true), - ); + let result = compile(&prql, &prql_compiler::Options::default().no_signature()); if lang_tags.contains(&LangTag::NoTest) { cmark_acc.push(Event::Html(table_of_prql_only(&prql).into())); @@ -309,6 +304,9 @@ this is an error ``` "###; + // Here we do want colors + anstream::ColorChoice::Always.write_global(); + assert_display_snapshot!(replace_examples(md)?, @r###" # PRQL Doc diff --git a/web/book/tests/documentation/main.rs b/web/book/tests/documentation/main.rs index ec27a274bdb3..fbd7ddd038ef 100644 --- a/web/book/tests/documentation/main.rs +++ b/web/book/tests/documentation/main.rs @@ -7,5 +7,6 @@ mod website; use ::prql_compiler::Options; fn compile(prql: &str) -> Result { + anstream::ColorChoice::Never.write_global(); prql_compiler::compile(prql, &Options::default().no_signature()) }