From 32ee4809424bd9dbd52c296036689f4fe712f720 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 4 Dec 2024 18:09:44 +0900 Subject: [PATCH] formatter: leverage serde to parse color styles Here we don't use an untagged enum to dispatch deserializer by type. An untagged enum wouldn't propagate underlying parse errors. --- CHANGELOG.md | 2 +- cli/src/formatter.rs | 120 +++++++++++++++++++++++++------------------ 2 files changed, 72 insertions(+), 50 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 295ecda3cb..5944025560 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). not converted to a string `"true"`, and vice versa. * The following configuration variables are now parsed strictly: - `ui.color`, `ui.progress-indicator`, `ui.quiet` + `colors.`, `ui.color`, `ui.progress-indicator`, `ui.quiet` * The deprecated `[alias]` config section is no longer respected. Move command aliases to the `[aliases]` section. diff --git a/cli/src/formatter.rs b/cli/src/formatter.rs index 11c648b74c..a58ad2073f 100644 --- a/cli/src/formatter.rs +++ b/cli/src/formatter.rs @@ -31,6 +31,9 @@ use crossterm::style::SetForegroundColor; use itertools::Itertools; use jj_lib::config::ConfigGetError; use jj_lib::config::StackedConfig; +use serde::de::Deserialize as _; +use serde::de::Error as _; +use serde::de::IntoDeserializer as _; // Lets the caller label strings and translates the labels to colors pub trait Formatter: Write { @@ -253,9 +256,12 @@ impl Formatter for SanitizingFormatter { } } -#[derive(Clone, Debug, Default, PartialEq, Eq)] +#[derive(Clone, Debug, Default, PartialEq, Eq, serde::Deserialize)] +#[serde(default, rename_all = "kebab-case")] pub struct Style { + #[serde(deserialize_with = "deserialize_color_opt")] pub fg: Option, + #[serde(deserialize_with = "deserialize_color_opt")] pub bg: Option, pub bold: Option, pub underline: Option, @@ -406,53 +412,48 @@ impl ColorFormatter { } fn rules_from_config(config: &StackedConfig) -> Result { - let mut result = vec![]; - for key in config.table_keys("colors") { - let to_config_err = |message: String| ConfigGetError::Type { - name: format!("colors.'{key}'"), - error: message.into(), - source_path: None, - }; - let labels = key - .split_whitespace() - .map(ToString::to_string) - .collect_vec(); - // TODO: report invalid type? - let value = config.get_value(["colors", key])?; - if let Some(color_name) = value.as_str() { - let style = Style { - fg: Some(color_for_name_or_hex(color_name).map_err(to_config_err)?), - bg: None, - bold: None, - underline: None, - }; - result.push((labels, style)); - } else if let Some(style_table) = value.as_inline_table() { - let mut style = Style::default(); - if let Some(item) = style_table.get("fg") { - if let Some(color_name) = item.as_str() { - style.fg = Some(color_for_name_or_hex(color_name).map_err(to_config_err)?); - } - } - if let Some(item) = style_table.get("bg") { - if let Some(color_name) = item.as_str() { - style.bg = Some(color_for_name_or_hex(color_name).map_err(to_config_err)?); - } - } - if let Some(item) = style_table.get("bold") { - if let Some(value) = item.as_bool() { - style.bold = Some(value); - } - } - if let Some(item) = style_table.get("underline") { - if let Some(value) = item.as_bool() { - style.underline = Some(value); + config + .table_keys("colors") + .map(|key| { + let labels = key + .split_whitespace() + .map(ToString::to_string) + .collect_vec(); + let style = config.get_value_with(["colors", key], |value| { + if value.is_str() { + Ok(Style { + fg: Some(deserialize_color(value.into_deserializer())?), + bg: None, + bold: None, + underline: None, + }) + } else if value.is_inline_table() { + Style::deserialize(value.into_deserializer()) + } else { + Err(toml_edit::de::Error::custom(format!( + "invalid type: {}, expected a color name or a table of styles", + value.type_name() + ))) } - } - result.push((labels, style)); - } - } - Ok(result) + })?; + Ok((labels, style)) + }) + .collect() +} + +fn deserialize_color<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + let name_or_hex = String::deserialize(deserializer)?; + color_for_name_or_hex(&name_or_hex).map_err(D::Error::custom) +} + +fn deserialize_color_opt<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + deserialize_color(deserializer).map(Some) } fn color_for_name_or_hex(name_or_hex: &str) -> Result { @@ -1024,7 +1025,7 @@ mod tests { ); let mut output: Vec = vec![]; let err = ColorFormatter::for_config(&mut output, &config, false).unwrap_err(); - insta::assert_snapshot!(err, @"Invalid type or value for colors.'outer inner'"); + insta::assert_snapshot!(err, @r#"Invalid type or value for colors."outer inner""#); insta::assert_snapshot!(err.source().unwrap(), @"Invalid color: bloo"); } @@ -1039,10 +1040,31 @@ mod tests { ); let mut output: Vec = vec![]; let err = ColorFormatter::for_config(&mut output, &config, false).unwrap_err(); - insta::assert_snapshot!(err, @"Invalid type or value for colors.'outer inner'"); + insta::assert_snapshot!(err, @r#"Invalid type or value for colors."outer inner""#); insta::assert_snapshot!(err.source().unwrap(), @"Invalid color: #ffgggg"); } + #[test] + fn test_color_formatter_invalid_type_of_color() { + let config = config_from_string("colors.foo = []"); + let err = ColorFormatter::for_config(&mut Vec::new(), &config, false).unwrap_err(); + insta::assert_snapshot!(err, @"Invalid type or value for colors.foo"); + insta::assert_snapshot!( + err.source().unwrap(), + @"invalid type: array, expected a color name or a table of styles"); + } + + #[test] + fn test_color_formatter_invalid_type_of_style() { + let config = config_from_string("colors.foo = { bold = 1 }"); + let err = ColorFormatter::for_config(&mut Vec::new(), &config, false).unwrap_err(); + insta::assert_snapshot!(err, @"Invalid type or value for colors.foo"); + insta::assert_snapshot!(err.source().unwrap(), @r" + invalid type: integer `1`, expected a boolean + in `bold` + "); + } + #[test] fn test_color_formatter_normal_color() { // The "default" color resets the color. It is possible to reset only the