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

refactor: Replace concolor with anstream & friends #2773

Merged
merged 15 commits into from
Jun 10, 2023
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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**:
Expand Down
57 changes: 15 additions & 42 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion bindings/prql-js/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use wasm_bindgen::prelude::*;
pub fn compile(prql_query: &str, options: Option<CompileOptions>) -> Option<String> {
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())),
)
}

Expand Down
2 changes: 1 addition & 1 deletion bindings/prql-lib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
2 changes: 1 addition & 1 deletion bindings/prql-python/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub fn compile(prql_query: &str, options: Option<CompileOptions>) -> PyResult<St
.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()))
.map_err(|e| (PyErr::new::<exceptions::PySyntaxError, _>(e.to_string())))
}

Expand Down
1 change: 1 addition & 0 deletions prql-compiler/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
5 changes: 3 additions & 2 deletions prql-compiler/prqlc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,16 @@ 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"
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"
Expand Down
23 changes: 10 additions & 13 deletions prql-compiler/prqlc/src/cli.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use anstream::eprintln;
use anyhow::bail;
use anyhow::Result;
use ariadne::Source;
Expand All @@ -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}");
Expand All @@ -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)]
Expand Down Expand Up @@ -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 } => {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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]
Expand Down
40 changes: 29 additions & 11 deletions prql-compiler/src/error.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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 {
Expand All @@ -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<String> {
let config = Config::default().with_color(color);
fn compose_display(&self, source_path: PathBuf, cache: &mut FileTreeCache) -> Option<String> {
// 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?);

Expand All @@ -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<SourceLocation> {
Expand All @@ -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<PathBuf, Source>,
Expand Down
Loading