From dde2a427978ca62a18bd0d5002ab3a07ff9a4c3a Mon Sep 17 00:00:00 2001 From: CosmicHorror Date: Thu, 13 Jul 2023 19:53:10 -0600 Subject: [PATCH] Make config code highlighter error more friendly (#152) * Change opts test layout * Show existing opaque error * Improve error message for config code highlighter --- src/color.rs | 86 +++++++++++++++++-- src/opts/tests/error_msg.rs | 24 ++++++ src/opts/tests/mod.rs | 2 + src/opts/{tests.rs => tests/parse.rs} | 10 ++- ...s__tests__error_msg__invalid_theme_ty.snap | 13 +++ ...opts__tests__error_msg__unknown_theme.snap | 11 +++ 6 files changed, 133 insertions(+), 13 deletions(-) create mode 100644 src/opts/tests/error_msg.rs create mode 100644 src/opts/tests/mod.rs rename src/opts/{tests.rs => tests/parse.rs} (96%) create mode 100644 src/opts/tests/snapshots/inlyne__opts__tests__error_msg__invalid_theme_ty.snap create mode 100644 src/opts/tests/snapshots/inlyne__opts__tests__error_msg__unknown_theme.snap diff --git a/src/color.rs b/src/color.rs index 38015dd4..f915d494 100644 --- a/src/color.rs +++ b/src/color.rs @@ -107,14 +107,21 @@ impl PartialEq for Theme { } } -// TODO: the error message here degraded when switching this to allow for custom themes. It'd be -// good to still list all the default themes when passing in something else. Add a test for -// the error message -#[derive(Deserialize, Clone, Debug, PartialEq, Eq)] -#[serde(untagged)] +#[derive(Clone, Debug, PartialEq, Eq)] pub enum SyntaxTheme { Defaults(ThemeDefaults), - Custom { path: PathBuf }, + Custom(ThemeCustom), +} + +#[derive(Deserialize, Clone, Debug, PartialEq, Eq)] +pub struct ThemeCustom { + path: PathBuf, +} + +impl SyntaxTheme { + pub fn custom(path: PathBuf) -> Self { + Self::Custom(ThemeCustom { path }) + } } impl TryFrom for SyntectTheme { @@ -123,7 +130,7 @@ impl TryFrom for SyntectTheme { fn try_from(syntax_theme: SyntaxTheme) -> Result { match syntax_theme { SyntaxTheme::Defaults(default) => Ok(SyntectTheme::from(default)), - SyntaxTheme::Custom { path } => { + SyntaxTheme::Custom(ThemeCustom { path }) => { let mut reader = BufReader::new(File::open(&path).with_context(|| { format!("Failed opening theme from path {}", path.display()) })?); @@ -134,8 +141,51 @@ impl TryFrom for SyntectTheme { } } -#[derive(Deserialize, Clone, Copy, Debug, PartialEq, Eq)] -#[serde(rename_all = "kebab-case")] +// Give better error messages than regular `#[serde(untagged)]` +impl<'de> Deserialize<'de> for SyntaxTheme { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + #[derive(Deserialize)] + #[serde(untagged)] + enum Untagged { + Defaults(String), + Custom(ThemeCustom), + } + + let Ok(untagged) = Untagged::deserialize(deserializer) else { + return Err(serde::de::Error::custom( + "Expects either a default theme name or a path to a custom theme. E.g.\n\ + default: \"inspired-github\"\n\ + custom: { path = \"/path/to/custom.tmTheme\" }" + )) + }; + + match untagged { + // Unfortunately #[serde(untagged)] uses private internals to reuse a deserializer + // mutliple times. We can't so now we have to fall back to other means to give a good + // error message ;-; + Untagged::Defaults(theme_name) => match ThemeDefaults::from_kebab(&theme_name) { + Some(theme) => Ok(Self::Defaults(theme)), + None => { + let variants = ThemeDefaults::kebab_pairs() + .iter() + .map(|(kebab, _)| format!("\"{kebab}\"")) + .collect::>() + .join(", "); + let msg = format!( + "\"{theme_name}\" didn't match any of the expected variants: [{variants}]" + ); + Err(serde::de::Error::custom(msg)) + } + }, + Untagged::Custom(custom) => Ok(Self::Custom(custom)), + } + } +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum ThemeDefaults { Base16OceanDark, Base16EightiesDark, @@ -147,6 +197,24 @@ pub enum ThemeDefaults { } impl ThemeDefaults { + fn kebab_pairs() -> &'static [(&'static str, Self)] { + &[ + ("base16-ocean-dark", Self::Base16OceanDark), + ("base16-eighties-dark", Self::Base16EightiesDark), + ("base16-mocha-dark", Self::Base16MochaDark), + ("base16-ocean-light", Self::Base16OceanLight), + ("inspired-github", Self::InspiredGithub), + ("solarized-dark", Self::SolarizedDark), + ("solarized-light", Self::SolarizedLight), + ] + } + + fn from_kebab(kebab: &str) -> Option { + Self::kebab_pairs() + .iter() + .find_map(|&(hay, var)| (kebab == hay).then_some(var)) + } + pub fn as_syntect_name(self) -> &'static str { match self { Self::Base16OceanDark => "base16-ocean.dark", diff --git a/src/opts/tests/error_msg.rs b/src/opts/tests/error_msg.rs new file mode 100644 index 00000000..68cb7ccd --- /dev/null +++ b/src/opts/tests/error_msg.rs @@ -0,0 +1,24 @@ +macro_rules! snapshot_config_parse_error { + ( $( ($test_name:ident, $md_text:ident) ),* $(,)? ) => { + $( + #[test] + fn $test_name() { + let err = ::toml::from_str::<$crate::opts::Config>($md_text).unwrap_err(); + + ::insta::with_settings!({ + description => $md_text, + }, { + ::insta::assert_display_snapshot!(err); + }); + } + )* + } +} + +const UNKNOWN_THEME: &str = r#"light-theme.code-highlighter = "doesnt-exist""#; +const INVALID_THEME_TY: &str = "light-theme.code-highlighter = []"; + +snapshot_config_parse_error!( + (unknown_theme, UNKNOWN_THEME), + (invalid_theme_ty, INVALID_THEME_TY), +); diff --git a/src/opts/tests/mod.rs b/src/opts/tests/mod.rs new file mode 100644 index 00000000..89416fb1 --- /dev/null +++ b/src/opts/tests/mod.rs @@ -0,0 +1,2 @@ +mod error_msg; +mod parse; diff --git a/src/opts/tests.rs b/src/opts/tests/parse.rs similarity index 96% rename from src/opts/tests.rs rename to src/opts/tests/parse.rs index d89e0655..2a5c1cf6 100644 --- a/src/opts/tests.rs +++ b/src/opts/tests/parse.rs @@ -1,10 +1,12 @@ use std::{ffi::OsString, path::PathBuf}; -use super::{cli, config, Opts, ResolvedTheme, ThemeType}; use crate::color::{SyntaxTheme, Theme, ThemeDefaults}; use crate::keybindings::Keybindings; -use crate::opts::config::{FontOptions, LinesToScroll}; -use crate::opts::Args; +use crate::opts::{ + cli, + config::{self, FontOptions, LinesToScroll}, + Args, Opts, ResolvedTheme, ThemeType, +}; use pretty_assertions::assert_eq; @@ -200,7 +202,7 @@ fn custom_syntax_theme() { fn config_with_theme_at(path: PathBuf) -> config::Config { let mut config = config::Config::default(); config.light_theme = Some(config::OptionalTheme { - code_highlighter: Some(SyntaxTheme::Custom { path }), + code_highlighter: Some(SyntaxTheme::custom(path)), ..Default::default() }); config diff --git a/src/opts/tests/snapshots/inlyne__opts__tests__error_msg__invalid_theme_ty.snap b/src/opts/tests/snapshots/inlyne__opts__tests__error_msg__invalid_theme_ty.snap new file mode 100644 index 00000000..4239d661 --- /dev/null +++ b/src/opts/tests/snapshots/inlyne__opts__tests__error_msg__invalid_theme_ty.snap @@ -0,0 +1,13 @@ +--- +source: src/opts/tests/error_msg.rs +description: "light-theme.code-highlighter = []" +expression: err +--- +TOML parse error at line 1, column 32 + | +1 | light-theme.code-highlighter = [] + | ^^ +Expects either a default theme name or a path to a custom theme. E.g. +default: "inspired-github" +custom: { path = "/path/to/custom.tmTheme" } + diff --git a/src/opts/tests/snapshots/inlyne__opts__tests__error_msg__unknown_theme.snap b/src/opts/tests/snapshots/inlyne__opts__tests__error_msg__unknown_theme.snap new file mode 100644 index 00000000..91886ab2 --- /dev/null +++ b/src/opts/tests/snapshots/inlyne__opts__tests__error_msg__unknown_theme.snap @@ -0,0 +1,11 @@ +--- +source: src/opts/tests/error_msg.rs +description: "light-theme.code-highlighter = \"doesnt-exist\"" +expression: err +--- +TOML parse error at line 1, column 32 + | +1 | light-theme.code-highlighter = "doesnt-exist" + | ^^^^^^^^^^^^^^ +"doesnt-exist" didn't match any of the expected variants: ["base16-ocean-dark", "base16-eighties-dark", "base16-mocha-dark", "base16-ocean-light", "inspired-github", "solarized-dark", "solarized-light"] +