From aec2425b0b72e7117377442065eeb079ff83393e Mon Sep 17 00:00:00 2001 From: Federico Terzi Date: Sun, 18 Jul 2021 12:10:56 +0200 Subject: [PATCH] feat(config): refactor config handler to also return warnings and errors --- espanso-config/src/config/mod.rs | 4 +- espanso-config/src/config/store.rs | 25 +- espanso-config/src/error.rs | 71 +++ espanso-config/src/legacy/mod.rs | 20 +- espanso-config/src/lib.rs | 140 +++++- .../src/matches/group/loader/mod.rs | 28 +- .../src/matches/group/loader/yaml/mod.rs | 445 ++++++++++-------- espanso-config/src/matches/group/mod.rs | 4 +- espanso-config/src/matches/group/path.rs | 43 +- espanso-config/src/matches/mod.rs | 32 +- espanso-config/src/matches/store/default.rs | 150 ++++-- espanso-config/src/matches/store/mod.rs | 7 +- 12 files changed, 682 insertions(+), 287 deletions(-) create mode 100644 espanso-config/src/error.rs diff --git a/espanso-config/src/config/mod.rs b/espanso-config/src/config/mod.rs index fe5a54d..3411da5 100644 --- a/espanso-config/src/config/mod.rs +++ b/espanso-config/src/config/mod.rs @@ -30,6 +30,8 @@ pub(crate) mod store; #[cfg(test)] use mockall::{automock, predicate::*}; + +use crate::error::NonFatalErrorSet; #[cfg_attr(test, automock)] pub trait Config: Send { fn id(&self) -> i32; @@ -141,7 +143,7 @@ pub enum ToggleKey { LeftMeta, } -pub fn load_store(config_dir: &Path) -> Result { +pub fn load_store(config_dir: &Path) -> Result<(impl ConfigStore, Vec)> { store::DefaultConfigStore::load(config_dir) } diff --git a/espanso-config/src/config/store.rs b/espanso-config/src/config/store.rs index e48222c..79ae976 100644 --- a/espanso-config/src/config/store.rs +++ b/espanso-config/src/config/store.rs @@ -17,8 +17,10 @@ * along with espanso. If not, see . */ +use crate::error::NonFatalErrorSet; + use super::{resolve::ResolvedConfig, Config, ConfigStore, ConfigStoreError}; -use anyhow::Result; +use anyhow::{Context, Result}; use log::{debug, error}; use std::{collections::HashSet, path::Path}; @@ -67,8 +69,7 @@ impl ConfigStore for DefaultConfigStore { } impl DefaultConfigStore { - // TODO: test - pub fn load(config_dir: &Path) -> Result { + pub fn load(config_dir: &Path) -> Result<(Self, Vec)> { if !config_dir.is_dir() { return Err(ConfigStoreError::InvalidConfigDir().into()); } @@ -78,7 +79,11 @@ impl DefaultConfigStore { if !default_file.exists() || !default_file.is_file() { return Err(ConfigStoreError::MissingDefault().into()); } - let default = ResolvedConfig::load(&default_file, None)?; + + let mut non_fatal_errors = Vec::new(); + + let default = + ResolvedConfig::load(&default_file, None).context("failed to load default configuration")?; debug!("loaded default config at path: {:?}", default_file); // Then the others @@ -107,15 +112,19 @@ impl DefaultConfigStore { "unable to load config at path: {:?}, with error: {}", config_file, err ); + non_fatal_errors.push(NonFatalErrorSet::single_error(&config_file, err)); } } } } - Ok(Self { - default: Box::new(default), - customs, - }) + Ok(( + Self { + default: Box::new(default), + customs, + }, + non_fatal_errors, + )) } pub fn from_configs(default: Box, customs: Vec>) -> Result { diff --git a/espanso-config/src/error.rs b/espanso-config/src/error.rs new file mode 100644 index 0000000..0001391 --- /dev/null +++ b/espanso-config/src/error.rs @@ -0,0 +1,71 @@ +/* + * This file is part of espanso. + * + * Copyright (C) 2019-2021 Federico Terzi + * + * espanso is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * espanso is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with espanso. If not, see . + */ + +use std::path::{Path, PathBuf}; +use anyhow::Error; + +#[derive(Debug)] +pub struct NonFatalErrorSet { + pub file: PathBuf, + pub errors: Vec, +} + +impl NonFatalErrorSet { + pub fn new(file: &Path, non_fatal_errors: Vec) -> Self { + Self { + file: file.to_owned(), + errors: non_fatal_errors, + } + } + + pub fn single_error(file: &Path, error: Error) -> Self { + Self { + file: file.to_owned(), + errors: vec![ErrorRecord::error(error)], + } + } +} + +#[derive(Debug)] +pub struct ErrorRecord { + pub level: ErrorLevel, + pub error: Error, +} + +impl ErrorRecord { + pub fn error(error: Error) -> Self { + Self { + level: ErrorLevel::Error, + error, + } + } + + pub fn warn(error: Error) -> Self { + Self { + level: ErrorLevel::Warning, + error, + } + } +} + +#[derive(Debug)] +pub enum ErrorLevel { + Error, + Warning, +} \ No newline at end of file diff --git a/espanso-config/src/legacy/mod.rs b/espanso-config/src/legacy/mod.rs index f8be156..d33b9b6 100644 --- a/espanso-config/src/legacy/mod.rs +++ b/espanso-config/src/legacy/mod.rs @@ -18,14 +18,12 @@ */ use anyhow::Result; +use log::warn; use regex::Regex; use std::{collections::HashMap, path::Path}; use self::config::LegacyConfig; -use crate::matches::{ - group::loader::yaml::parse::{YAMLMatch, YAMLVariable}, - MatchEffect, -}; +use crate::matches::{MatchEffect, group::loader::yaml::{parse::{YAMLMatch, YAMLVariable}, try_convert_into_match, try_convert_into_variable}}; use crate::{config::store::DefaultConfigStore, counter::StructId}; use crate::{ config::Config, @@ -85,7 +83,10 @@ fn split_config(config: LegacyConfig) -> (LegacyInteropConfig, LegacyMatchGroup) .iter() .filter_map(|var| { let var: YAMLVariable = serde_yaml::from_value(var.clone()).ok()?; - let var: Variable = var.try_into().ok()?; + let (var, warnings) = try_convert_into_variable(var).ok()?; + warnings.into_iter().for_each(|warning| { + warn!("{}", warning); + }); Some(var) }) .collect(); @@ -95,7 +96,10 @@ fn split_config(config: LegacyConfig) -> (LegacyInteropConfig, LegacyMatchGroup) .iter() .filter_map(|var| { let m: YAMLMatch = serde_yaml::from_value(var.clone()).ok()?; - let m: Match = m.try_into().ok()?; + let (m, warnings) = try_convert_into_match(m).ok()?; + warnings.into_iter().for_each(|warning| { + warn!("{}", warning); + }); Some(m) }) .collect(); @@ -376,6 +380,10 @@ impl MatchStore for LegacyMatchStore { } } } + + fn loaded_paths(&self) -> Vec { + self.groups.keys().map(|key| key.clone()).collect() + } } #[cfg(test)] diff --git a/espanso-config/src/lib.rs b/espanso-config/src/lib.rs index b61a342..2293fe8 100644 --- a/espanso-config/src/lib.rs +++ b/espanso-config/src/lib.rs @@ -28,22 +28,27 @@ extern crate lazy_static; pub mod config; mod counter; +pub mod error; mod legacy; pub mod matches; mod util; -pub fn load(base_path: &Path) -> Result<(Box, Box)> { +pub fn load(base_path: &Path) -> Result<(Box, Box, Vec)> { let config_dir = base_path.join("config"); if !config_dir.exists() || !config_dir.is_dir() { return Err(ConfigError::MissingConfigDir().into()); } - let config_store = config::load_store(&config_dir)?; + let (config_store, non_fatal_config_errors) = config::load_store(&config_dir)?; let root_paths = config_store.get_all_match_paths(); - let match_store = matches::store::new(&root_paths.into_iter().collect::>()); + let (match_store, non_fatal_match_errors) = matches::store::load(&root_paths.into_iter().collect::>()); - Ok((Box::new(config_store), Box::new(match_store))) + let mut non_fatal_errors = Vec::new(); + non_fatal_errors.extend(non_fatal_config_errors.into_iter()); + non_fatal_errors.extend(non_fatal_match_errors.into_iter()); + + Ok((Box::new(config_store), Box::new(match_store), non_fatal_errors)) } pub fn load_legacy(config_dir: &Path, package_dir: &Path) -> Result<(Box, Box)> { @@ -120,8 +125,9 @@ mod tests { ) .unwrap(); - let (config_store, match_store) = load(&base).unwrap(); + let (config_store, match_store, errors) = load(&base).unwrap(); + assert_eq!(errors.len(), 0); assert_eq!(config_store.default().match_paths().len(), 2); assert_eq!( config_store @@ -160,6 +166,130 @@ mod tests { }); } + #[test] + fn load_non_fatal_errors() { + use_test_directory(|base, match_dir, config_dir| { + let base_file = match_dir.join("base.yml"); + std::fs::write( + &base_file, + r#" + matches: + - "invalid"invalid + "#, + ) + .unwrap(); + + let another_file = match_dir.join("another.yml"); + std::fs::write( + &another_file, + r#" + imports: + - "_sub.yml" + + matches: + - trigger: "hello2" + replace: "world2" + "#, + ) + .unwrap(); + + let under_file = match_dir.join("_sub.yml"); + std::fs::write( + &under_file, + r#" + matches: + - trigger: "hello3" + replace: "world3"invalid + "#, + ) + .unwrap(); + + let config_file = config_dir.join("default.yml"); + std::fs::write(&config_file, r#""#).unwrap(); + + let custom_config_file = config_dir.join("custom.yml"); + std::fs::write( + &custom_config_file, + r#" + filter_title: "Chrome" + " + + use_standard_includes: false + includes: ["../match/another.yml"] + "#, + ) + .unwrap(); + + let (config_store, match_store, errors) = load(&base).unwrap(); + + assert_eq!(errors.len(), 3); + // It shouldn't have loaded the "config.yml" one because of the YAML error + assert_eq!(config_store.configs().len(), 1); + // It shouldn't load "base.yml" and "_sub.yml" due to YAML errors + assert_eq!(match_store.loaded_paths().len(), 1); + }); + } + + #[test] + fn load_non_fatal_match_errors() { + use_test_directory(|base, match_dir, config_dir| { + let base_file = match_dir.join("base.yml"); + std::fs::write( + &base_file, + r#" + matches: + - trigger: "hello" + replace: "world" + - trigger: "invalid because there is no action field" + "#, + ) + .unwrap(); + + let config_file = config_dir.join("default.yml"); + std::fs::write(&config_file, r#""#).unwrap(); + + let (config_store, match_store, errors) = load(&base).unwrap(); + + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].file, base_file); + assert_eq!(errors[0].errors.len(), 1); + + assert_eq!( + match_store + .query(config_store.default().match_paths()) + .matches + .len(), + 1 + ); + }); + } + + #[test] + fn load_fatal_errors() { + use_test_directory(|base, match_dir, config_dir| { + let base_file = match_dir.join("base.yml"); + std::fs::write( + &base_file, + r#" + matches: + - trigger: hello + replace: world + "#, + ) + .unwrap(); + + let config_file = config_dir.join("default.yml"); + std::fs::write(&config_file, r#" + invalid + + " + "#).unwrap(); + + // A syntax error in the default.yml file cannot be handled gracefully + assert!(load(&base).is_err()); + }); + } + #[test] fn load_without_valid_config_dir() { use_test_directory(|_, match_dir, _| { diff --git a/espanso-config/src/matches/group/loader/mod.rs b/espanso-config/src/matches/group/loader/mod.rs index 574269a..ed32c66 100644 --- a/espanso-config/src/matches/group/loader/mod.rs +++ b/espanso-config/src/matches/group/loader/mod.rs @@ -21,6 +21,8 @@ use anyhow::Result; use std::path::Path; use thiserror::Error; +use crate::error::NonFatalErrorSet; + use self::yaml::YAMLImporter; use super::MatchGroup; @@ -29,14 +31,14 @@ pub(crate) mod yaml; trait Importer { fn is_supported(&self, extension: &str) -> bool; - fn load_group(&self, path: &Path) -> Result; + fn load_group(&self, path: &Path) -> Result<(MatchGroup, Option)>; } lazy_static! { static ref IMPORTERS: Vec> = vec![Box::new(YAMLImporter::new()),]; } -pub(crate) fn load_match_group(path: &Path) -> Result { +pub(crate) fn load_match_group(path: &Path) -> Result<(MatchGroup, Option)> { if let Some(extension) = path.extension() { let extension = extension.to_string_lossy().to_lowercase(); @@ -46,25 +48,25 @@ pub(crate) fn load_match_group(path: &Path) -> Result { match importer { Some(importer) => match importer.load_group(path) { - Ok(group) => Ok(group), + Ok((group, non_fatal_error_set)) => Ok((group, non_fatal_error_set)), Err(err) => Err(LoadError::ParsingError(err).into()), }, - None => Err(LoadError::InvalidFormat().into()), + None => Err(LoadError::InvalidFormat.into()), } } else { - Err(LoadError::MissingExtension().into()) + Err(LoadError::MissingExtension.into()) } } #[derive(Error, Debug)] pub enum LoadError { #[error("missing extension in match group file")] - MissingExtension(), + MissingExtension, #[error("invalid match group format")] - InvalidFormat(), + InvalidFormat, - #[error("parser reported an error: `{0}`")] + #[error(transparent)] ParsingError(anyhow::Error), } @@ -84,7 +86,7 @@ mod tests { .unwrap_err() .downcast::() .unwrap(), - LoadError::InvalidFormat() + LoadError::InvalidFormat )); }); } @@ -100,7 +102,7 @@ mod tests { .unwrap_err() .downcast::() .unwrap(), - LoadError::MissingExtension() + LoadError::MissingExtension )); }); } @@ -135,7 +137,7 @@ mod tests { ) .unwrap(); - assert_eq!(load_match_group(&file).unwrap().matches.len(), 1); + assert_eq!(load_match_group(&file).unwrap().0.matches.len(), 1); }); } @@ -153,7 +155,7 @@ mod tests { ) .unwrap(); - assert_eq!(load_match_group(&file).unwrap().matches.len(), 1); + assert_eq!(load_match_group(&file).unwrap().0.matches.len(), 1); }); } @@ -171,7 +173,7 @@ mod tests { ) .unwrap(); - assert_eq!(load_match_group(&file).unwrap().matches.len(), 1); + assert_eq!(load_match_group(&file).unwrap().0.matches.len(), 1); }); } } diff --git a/espanso-config/src/matches/group/loader/yaml/mod.rs b/espanso-config/src/matches/group/loader/yaml/mod.rs index fc66d5e..41da808 100644 --- a/espanso-config/src/matches/group/loader/yaml/mod.rs +++ b/espanso-config/src/matches/group/loader/yaml/mod.rs @@ -19,17 +19,16 @@ use crate::{ counter::next_id, + error::{ErrorRecord, NonFatalErrorSet}, matches::{ group::{path::resolve_imports, MatchGroup}, ImageEffect, Match, Params, RegexCause, TextFormat, TextInjectMode, UpperCasingStyle, Value, Variable, }, }; -use anyhow::Result; -use log::{error, warn}; +use anyhow::{anyhow, bail, Context, Result}; use parse::YAMLMatchGroup; use regex::{Captures, Regex}; -use std::convert::{TryFrom, TryInto}; use self::{ parse::{YAMLMatch, YAMLVariable}, @@ -46,6 +45,9 @@ lazy_static! { static ref VAR_REGEX: Regex = Regex::new("\\{\\{\\s*(\\w+)(\\.\\w+)?\\s*\\}\\}").unwrap(); } +// Create an alias to make the meaning more explicit +type Warning = anyhow::Error; + pub(crate) struct YAMLImporter {} impl YAMLImporter { @@ -62,203 +64,228 @@ impl Importer for YAMLImporter { fn load_group( &self, path: &std::path::Path, - ) -> anyhow::Result { - let yaml_group = YAMLMatchGroup::parse_from_file(path)?; + ) -> anyhow::Result<(crate::matches::group::MatchGroup, Option)> { + let yaml_group = + YAMLMatchGroup::parse_from_file(path).context("failed to parse YAML match group")?; - let global_vars: Result> = yaml_group - .global_vars - .as_ref() - .cloned() - .unwrap_or_default() - .iter() - .map(|var| var.clone().try_into()) - .collect(); + let mut non_fatal_errors = Vec::new(); - let matches: Result> = yaml_group - .matches - .as_ref() - .cloned() - .unwrap_or_default() - .iter() - .map(|m| m.clone().try_into()) - .collect(); + let mut global_vars = Vec::new(); + for yaml_global_var in yaml_group.global_vars.as_ref().cloned().unwrap_or_default() { + match try_convert_into_variable(yaml_global_var) { + Ok((var, warnings)) => { + global_vars.push(var); + non_fatal_errors.extend(warnings.into_iter().map(ErrorRecord::warn)); + } + Err(err) => { + non_fatal_errors.push(ErrorRecord::error(err)); + } + } + } + + let mut matches = Vec::new(); + for yaml_match in yaml_group.matches.as_ref().cloned().unwrap_or_default() { + match try_convert_into_match(yaml_match) { + Ok((m, warnings)) => { + matches.push(m); + non_fatal_errors.extend(warnings.into_iter().map(ErrorRecord::warn)); + } + Err(err) => { + non_fatal_errors.push(ErrorRecord::error(err)); + } + } + } // Resolve imports - let resolved_imports = resolve_imports(path, &yaml_group.imports.unwrap_or_default())?; + let (resolved_imports, import_errors) = + resolve_imports(path, &yaml_group.imports.unwrap_or_default()) + .context("failed to resolve YAML match group imports")?; + non_fatal_errors.extend(import_errors); - Ok(MatchGroup { - imports: resolved_imports, - global_vars: global_vars?, - matches: matches?, - }) + let non_fatal_error_set = if !non_fatal_errors.is_empty() { + Some(NonFatalErrorSet::new(path, non_fatal_errors)) + } else { + None + }; + + Ok(( + MatchGroup { + imports: resolved_imports, + global_vars: global_vars, + matches: matches, + }, + non_fatal_error_set, + )) } } -impl TryFrom for Match { - type Error = anyhow::Error; +pub fn try_convert_into_match(yaml_match: YAMLMatch) -> Result<(Match, Vec)> { + let mut warnings = Vec::new(); - fn try_from(yaml_match: YAMLMatch) -> Result { - if yaml_match.uppercase_style.is_some() && yaml_match.propagate_case.is_none() { - warn!("specifying the 'uppercase_style' option without 'propagate_case' has no effect"); + if yaml_match.uppercase_style.is_some() && yaml_match.propagate_case.is_none() { + warnings.push(anyhow!( + "specifying the 'uppercase_style' option without 'propagate_case' has no effect" + )); + } + + let triggers = if let Some(trigger) = yaml_match.trigger { + Some(vec![trigger]) + } else if let Some(triggers) = yaml_match.triggers { + Some(triggers) + } else { + None + }; + + let uppercase_style = match yaml_match + .uppercase_style + .map(|s| s.to_lowercase()) + .as_deref() + { + Some("uppercase") => UpperCasingStyle::Uppercase, + Some("capitalize") => UpperCasingStyle::Capitalize, + Some("capitalize_words") => UpperCasingStyle::CapitalizeWords, + Some(style) => { + warnings.push(anyhow!( + "unrecognized uppercase_style: {:?}, falling back to the default", + style + )); + TriggerCause::default().uppercase_style } + _ => TriggerCause::default().uppercase_style, + }; - let triggers = if let Some(trigger) = yaml_match.trigger { - Some(vec![trigger]) - } else if let Some(triggers) = yaml_match.triggers { - Some(triggers) - } else { - None - }; + let cause = if let Some(triggers) = triggers { + MatchCause::Trigger(TriggerCause { + triggers, + left_word: yaml_match + .left_word + .or(yaml_match.word) + .unwrap_or(TriggerCause::default().left_word), + right_word: yaml_match + .right_word + .or(yaml_match.word) + .unwrap_or(TriggerCause::default().right_word), + propagate_case: yaml_match + .propagate_case + .unwrap_or(TriggerCause::default().propagate_case), + uppercase_style, + }) + } else if let Some(regex) = yaml_match.regex { + // TODO: add test case + MatchCause::Regex(RegexCause { regex }) + } else { + MatchCause::None + }; - let uppercase_style = match yaml_match - .uppercase_style - .map(|s| s.to_lowercase()) - .as_deref() - { - Some("uppercase") => UpperCasingStyle::Uppercase, - Some("capitalize") => UpperCasingStyle::Capitalize, - Some("capitalize_words") => UpperCasingStyle::CapitalizeWords, - Some(style) => { - error!( - "unrecognized uppercase_style: {:?}, falling back to the default", - style - ); - TriggerCause::default().uppercase_style - } - _ => TriggerCause::default().uppercase_style, - }; + // TODO: test force_mode/force_clipboard + let force_mode = if let Some(true) = yaml_match.force_clipboard { + Some(TextInjectMode::Clipboard) + } else if let Some(mode) = yaml_match.force_mode { + match mode.to_lowercase().as_str() { + "clipboard" => Some(TextInjectMode::Clipboard), + "keys" => Some(TextInjectMode::Keys), + _ => None, + } + } else { + None + }; - let cause = if let Some(triggers) = triggers { - MatchCause::Trigger(TriggerCause { - triggers, - left_word: yaml_match - .left_word - .or(yaml_match.word) - .unwrap_or(TriggerCause::default().left_word), - right_word: yaml_match - .right_word - .or(yaml_match.word) - .unwrap_or(TriggerCause::default().right_word), - propagate_case: yaml_match - .propagate_case - .unwrap_or(TriggerCause::default().propagate_case), - uppercase_style, - }) - } else if let Some(regex) = yaml_match.regex { - // TODO: add test case - MatchCause::Regex(RegexCause { regex }) - } else { - MatchCause::None - }; - - // TODO: test force_mode/force_clipboard - let force_mode = if let Some(true) = yaml_match.force_clipboard { - Some(TextInjectMode::Clipboard) - } else if let Some(mode) = yaml_match.force_mode { - match mode.to_lowercase().as_str() { - "clipboard" => Some(TextInjectMode::Clipboard), - "keys" => Some(TextInjectMode::Keys), - _ => None, - } - } else { - None - }; - - let effect = - if yaml_match.replace.is_some() || yaml_match.markdown.is_some() || yaml_match.html.is_some() - { - // TODO: test markdown and html cases - let (replace, format) = if let Some(plain) = yaml_match.replace { - (plain, TextFormat::Plain) - } else if let Some(markdown) = yaml_match.markdown { - (markdown, TextFormat::Markdown) - } else if let Some(html) = yaml_match.html { - (html, TextFormat::Html) - } else { - unreachable!(); - }; - - let vars: Result> = yaml_match - .vars - .unwrap_or_default() - .into_iter() - .map(|var| var.try_into()) - .collect(); - - MatchEffect::Text(TextEffect { - replace, - vars: vars?, - format, - force_mode, - }) - } else if let Some(form_layout) = yaml_match.form { - // TODO: test form case - // Replace all the form fields with actual variables - let resolved_layout = VAR_REGEX - .replace_all(&form_layout, |caps: &Captures| { - let var_name = caps.get(1).unwrap().as_str(); - format!("{{{{form1.{}}}}}", var_name) - }) - .to_string(); - - // Convert escaped brakets in forms - let resolved_layout = resolved_layout.replace("\\{", "{ ").replace("\\}", " }"); - - // Convert the form data to valid variables - let mut params = Params::new(); - params.insert("layout".to_string(), Value::String(form_layout)); - - if let Some(fields) = yaml_match.form_fields { - params.insert("fields".to_string(), Value::Object(convert_params(fields)?)); - } - - let vars = vec![Variable { - id: next_id(), - name: "form1".to_owned(), - var_type: "form".to_owned(), - params, - }]; - - MatchEffect::Text(TextEffect { - replace: resolved_layout, - vars, - format: TextFormat::Plain, - force_mode, - }) - } else if let Some(image_path) = yaml_match.image_path { - // TODO: test image case - MatchEffect::Image(ImageEffect { path: image_path }) + let effect = + if yaml_match.replace.is_some() || yaml_match.markdown.is_some() || yaml_match.html.is_some() { + // TODO: test markdown and html cases + let (replace, format) = if let Some(plain) = yaml_match.replace { + (plain, TextFormat::Plain) + } else if let Some(markdown) = yaml_match.markdown { + (markdown, TextFormat::Markdown) + } else if let Some(html) = yaml_match.html { + (html, TextFormat::Html) } else { - MatchEffect::None + unreachable!(); }; - if let MatchEffect::None = effect { - warn!( - "match caused by {:?} does not produce any effect. Did you forget the 'replace' field?", - cause - ); - } + let mut vars: Vec = Vec::new(); + for yaml_var in yaml_match.vars.unwrap_or_default() { + let (var, var_warnings) = try_convert_into_variable(yaml_var.clone()) + .with_context(|| format!("failed to load variable: {:?}", yaml_var))?; + warnings.extend(var_warnings); + vars.push(var); + } - Ok(Self { + MatchEffect::Text(TextEffect { + replace, + vars, + format, + force_mode, + }) + } else if let Some(form_layout) = yaml_match.form { + // TODO: test form case + // Replace all the form fields with actual variables + let resolved_layout = VAR_REGEX + .replace_all(&form_layout, |caps: &Captures| { + let var_name = caps.get(1).unwrap().as_str(); + format!("{{{{form1.{}}}}}", var_name) + }) + .to_string(); + + // Convert escaped brakets in forms + let resolved_layout = resolved_layout.replace("\\{", "{ ").replace("\\}", " }"); + + // Convert the form data to valid variables + let mut params = Params::new(); + params.insert("layout".to_string(), Value::String(form_layout)); + + if let Some(fields) = yaml_match.form_fields { + params.insert("fields".to_string(), Value::Object(convert_params(fields)?)); + } + + let vars = vec![Variable { + id: next_id(), + name: "form1".to_owned(), + var_type: "form".to_owned(), + params, + }]; + + MatchEffect::Text(TextEffect { + replace: resolved_layout, + vars, + format: TextFormat::Plain, + force_mode, + }) + } else if let Some(image_path) = yaml_match.image_path { + // TODO: test image case + MatchEffect::Image(ImageEffect { path: image_path }) + } else { + MatchEffect::None + }; + + if let MatchEffect::None = effect { + bail!( + "match triggered by {:?} does not produce any effect. Did you forget the 'replace' field?", + cause.long_description() + ); + } + + Ok(( + Match { cause, effect, label: None, id: next_id(), - }) - } + }, + warnings, + )) } -impl TryFrom for Variable { - type Error = anyhow::Error; - - fn try_from(yaml_var: YAMLVariable) -> Result { - Ok(Self { +pub fn try_convert_into_variable(yaml_var: YAMLVariable) -> Result<(Variable, Vec)> { + Ok(( + Variable { name: yaml_var.name, var_type: yaml_var.var_type, params: convert_params(yaml_var.params)?, id: next_id(), - }) - } + }, + Vec::new(), + )) } #[cfg(test)] @@ -270,9 +297,9 @@ mod tests { }; use std::fs::create_dir_all; - fn create_match(yaml: &str) -> Result { + fn create_match_with_warnings(yaml: &str) -> Result<(Match, Vec)> { let yaml_match: YAMLMatch = serde_yaml::from_str(yaml)?; - let mut m: Match = yaml_match.try_into()?; + let (mut m, warnings) = try_convert_into_match(yaml_match)?; // Reset the IDs to correctly compare them m.id = 0; @@ -280,6 +307,14 @@ mod tests { e.vars.iter_mut().for_each(|v| v.id = 0); } + Ok((m, warnings)) + } + + fn create_match(yaml: &str) -> Result { + let (m, warnings) = create_match_with_warnings(yaml)?; + if !warnings.is_empty() { + panic!("warnings were detected but not handled: {:?}", warnings); + } Ok(m) } @@ -444,6 +479,7 @@ mod tests { trigger: "Hello" replace: "world" uppercase_style: "capitalize" + propagate_case: true "# ) .unwrap() @@ -460,6 +496,7 @@ mod tests { trigger: "Hello" replace: "world" uppercase_style: "capitalize_words" + propagate_case: true "# ) .unwrap() @@ -476,6 +513,7 @@ mod tests { trigger: "Hello" replace: "world" uppercase_style: "uppercase" + propagate_case: true "# ) .unwrap() @@ -486,21 +524,36 @@ mod tests { UpperCasingStyle::Uppercase, ); + // Invalid without propagate_case + let (m, warnings) = create_match_with_warnings( + r#" + trigger: "Hello" + replace: "world" + uppercase_style: "capitalize" + "#, + ) + .unwrap(); assert_eq!( - create_match( - r#" + m.cause.into_trigger().unwrap().uppercase_style, + UpperCasingStyle::Capitalize, + ); + assert_eq!(warnings.len(), 1); + + // Invalid style + let (m, warnings) = create_match_with_warnings( + r#" trigger: "Hello" replace: "world" uppercase_style: "invalid" - "# - ) - .unwrap() - .cause - .into_trigger() - .unwrap() - .uppercase_style, + propagate_case: true + "#, + ) + .unwrap(); + assert_eq!( + m.cause.into_trigger().unwrap().uppercase_style, UpperCasingStyle::Uppercase, ); + assert_eq!(warnings.len(), 1); } #[test] @@ -612,7 +665,10 @@ mod tests { std::fs::write(&sub_file, "").unwrap(); let importer = YAMLImporter::new(); - let mut group = importer.load_group(&base_file).unwrap(); + let (mut group, non_fatal_error_set) = importer.load_group(&base_file).unwrap(); + // The invalid import path should be reported as error + assert_eq!(non_fatal_error_set.unwrap().errors.len(), 1); + // Reset the ids to compare them correctly group.matches.iter_mut().for_each(|mut m| m.id = 0); group.global_vars.iter_mut().for_each(|mut v| v.id = 0); @@ -644,4 +700,23 @@ mod tests { ) }); } + + #[test] + fn importer_invalid_syntax() { + use_test_directory(|_, match_dir, _| { + let base_file = match_dir.join("base.yml"); + std::fs::write( + &base_file, + r#" + imports: + - invalid + - indentation + "#, + ) + .unwrap(); + + let importer = YAMLImporter::new(); + assert!(importer.load_group(&base_file).is_err()); + }) + } } diff --git a/espanso-config/src/matches/group/mod.rs b/espanso-config/src/matches/group/mod.rs index ae7aff0..3d61c23 100644 --- a/espanso-config/src/matches/group/mod.rs +++ b/espanso-config/src/matches/group/mod.rs @@ -20,6 +20,8 @@ use anyhow::Result; use std::path::Path; +use crate::error::NonFatalErrorSet; + use super::{Match, Variable}; pub(crate) mod loader; @@ -44,7 +46,7 @@ impl Default for MatchGroup { impl MatchGroup { // TODO: test - pub fn load(group_path: &Path) -> Result { + pub fn load(group_path: &Path) -> Result<(Self, Option)> { loader::load_match_group(group_path) } } diff --git a/espanso-config/src/matches/group/path.rs b/espanso-config/src/matches/group/path.rs index 35fd40f..01fc94e 100644 --- a/espanso-config/src/matches/group/path.rs +++ b/espanso-config/src/matches/group/path.rs @@ -17,12 +17,16 @@ * along with espanso. If not, see . */ -use anyhow::Result; -use log::error; +use anyhow::{anyhow, Context, Result}; use std::path::{Path, PathBuf}; use thiserror::Error; -pub fn resolve_imports(group_path: &Path, imports: &[String]) -> Result> { +use crate::error::ErrorRecord; + +pub fn resolve_imports( + group_path: &Path, + imports: &[String], +) -> Result<(Vec, Vec)> { let mut paths = Vec::new(); // Get the containing directory @@ -42,6 +46,8 @@ pub fn resolve_imports(group_path: &Path, imports: &[String]) -> Result Result { if canonical_path.exists() && canonical_path.is_file() { paths.push(canonical_path) } else { // Best effort imports - error!("unable to resolve import at path: {:?}", canonical_path); + non_fatal_errors.push(ErrorRecord::error(anyhow!( + "unable to resolve import at path: {:?}", + canonical_path + ))) } } - Err(error) => { - error!( - "unable to canonicalize import path: {:?}, with error: {}", - full_path, error - ); - } + Err(error) => non_fatal_errors.push(ErrorRecord::error(error)), } } @@ -75,7 +81,7 @@ pub fn resolve_imports(group_path: &Path, imports: &[String]) -> Result(&'a self) -> Option<&'a str> { - if let MatchCause::Trigger(trigger_cause) = &self.cause { - trigger_cause.triggers.first().map(|s| s.as_str()) - } else { - None - } - // TODO: insert rendering for hotkey/shortcut - // TODO: insert rendering for regex? I'm worried it might be too long + self.cause.description() } } @@ -84,6 +78,30 @@ pub enum MatchCause { // TODO: shortcut } +impl MatchCause { + // TODO: test + pub fn description<'a>(&'a self) -> Option<&'a str> { + if let MatchCause::Trigger(trigger_cause) = &self { + trigger_cause.triggers.first().map(|s| s.as_str()) + } else { + None + } + // TODO: insert rendering for hotkey/shortcut + // TODO: insert rendering for regex? I'm worried it might be too long + } + + // TODO: test + pub fn long_description<'a>(&'a self) -> String { + if let MatchCause::Trigger(trigger_cause) = &self { + format!("triggers: {:?}", trigger_cause.triggers) + } else { + "No description available".to_owned() + } + // TODO: insert rendering for hotkey/shortcut + // TODO: insert rendering for regex? I'm worried it might be too long + } +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct TriggerCause { pub triggers: Vec, diff --git a/espanso-config/src/matches/store/default.rs b/espanso-config/src/matches/store/default.rs index 3206585..2c346d5 100644 --- a/espanso-config/src/matches/store/default.rs +++ b/espanso-config/src/matches/store/default.rs @@ -17,32 +17,33 @@ * along with espanso. If not, see . */ -use log::error; -use std::{ - collections::{HashMap, HashSet}, - path::PathBuf, -}; - use super::{MatchSet, MatchStore}; use crate::{ counter::StructId, + error::NonFatalErrorSet, matches::{group::MatchGroup, Match, Variable}, }; +use anyhow::Context; +use std::{ + collections::{HashMap, HashSet}, + path::PathBuf, +}; pub(crate) struct DefaultMatchStore { pub groups: HashMap, } impl DefaultMatchStore { - pub fn new(paths: &[String]) -> Self { + pub fn load(paths: &[String]) -> (Self, Vec) { let mut groups = HashMap::new(); + let mut non_fatal_error_sets = Vec::new(); // Because match groups can imports other match groups, // we have to load them recursively starting from the // top-level ones. - load_match_groups_recursively(&mut groups, paths); + load_match_groups_recursively(&mut groups, paths, &mut non_fatal_error_sets); - Self { groups } + (Self { groups }, non_fatal_error_sets) } } @@ -69,21 +70,35 @@ impl MatchStore for DefaultMatchStore { global_vars, } } + + fn loaded_paths(&self) -> Vec { + self.groups.keys().map(|key| key.clone()).collect() + } } -fn load_match_groups_recursively(groups: &mut HashMap, paths: &[String]) { +fn load_match_groups_recursively( + groups: &mut HashMap, + paths: &[String], + non_fatal_error_sets: &mut Vec, +) { for path in paths.iter() { if !groups.contains_key(path) { let group_path = PathBuf::from(path); - match MatchGroup::load(&group_path) { - Ok(group) => { + match MatchGroup::load(&group_path) + .with_context(|| format!("unable to load match group {:?}", group_path)) + { + Ok((group, non_fatal_error_set)) => { let imports = group.imports.clone(); groups.insert(path.clone(), group); - load_match_groups_recursively(groups, &imports); + if let Some(non_fatal_error_set) = non_fatal_error_set { + non_fatal_error_sets.push(non_fatal_error_set); + } + + load_match_groups_recursively(groups, &imports, non_fatal_error_sets); } - Err(error) => { - error!("unable to load match group: {:?}", error); + Err(err) => { + non_fatal_error_sets.push(NonFatalErrorSet::single_error(&group_path, err)); } } } @@ -221,8 +236,9 @@ mod tests { ) .unwrap(); - let match_store = DefaultMatchStore::new(&[base_file.to_string_lossy().to_string()]); - + let (match_store, non_fatal_error_sets) = + DefaultMatchStore::load(&[base_file.to_string_lossy().to_string()]); + assert_eq!(non_fatal_error_sets.len(), 0); assert_eq!(match_store.groups.len(), 3); let base_group = &match_store @@ -230,11 +246,14 @@ mod tests { .get(&base_file.to_string_lossy().to_string()) .unwrap() .matches; - let base_group: Vec = base_group.iter().map(|m| { - let mut copy = m.clone(); - copy.id = 0; - copy - }).collect(); + let base_group: Vec = base_group + .iter() + .map(|m| { + let mut copy = m.clone(); + copy.id = 0; + copy + }) + .collect(); assert_eq!(base_group, create_matches(&[("hello", "world")])); @@ -243,11 +262,14 @@ mod tests { .get(&another_file.to_string_lossy().to_string()) .unwrap() .matches; - let another_group: Vec = another_group.iter().map(|m| { - let mut copy = m.clone(); - copy.id = 0; - copy - }).collect(); + let another_group: Vec = another_group + .iter() + .map(|m| { + let mut copy = m.clone(); + copy.id = 0; + copy + }) + .collect(); assert_eq!( another_group, create_matches(&[("hello", "world2"), ("foo", "bar")]) @@ -258,11 +280,14 @@ mod tests { .get(&sub_file.to_string_lossy().to_string()) .unwrap() .matches; - let sub_group: Vec = sub_group.iter().map(|m| { - let mut copy = m.clone(); - copy.id = 0; - copy - }).collect(); + let sub_group: Vec = sub_group + .iter() + .map(|m| { + let mut copy = m.clone(); + copy.id = 0; + copy + }) + .collect(); assert_eq!(sub_group, create_matches(&[("hello", "world3")])); }); } @@ -317,9 +342,11 @@ mod tests { ) .unwrap(); - let match_store = DefaultMatchStore::new(&[base_file.to_string_lossy().to_string()]); + let (match_store, non_fatal_error_sets) = + DefaultMatchStore::load(&[base_file.to_string_lossy().to_string()]); assert_eq!(match_store.groups.len(), 3); + assert_eq!(non_fatal_error_sets.len(), 0); }); } @@ -378,7 +405,9 @@ mod tests { ) .unwrap(); - let match_store = DefaultMatchStore::new(&[base_file.to_string_lossy().to_string()]); + let (match_store, non_fatal_error_sets) = + DefaultMatchStore::load(&[base_file.to_string_lossy().to_string()]); + assert_eq!(non_fatal_error_sets.len(), 0); let match_set = match_store.query(&[base_file.to_string_lossy().to_string()]); @@ -387,7 +416,10 @@ mod tests { .matches .into_iter() .cloned() - .map(|mut m| { m.id = 0; m }) + .map(|mut m| { + m.id = 0; + m + }) .collect::>(), create_matches(&[ ("hello", "world3"), @@ -402,7 +434,10 @@ mod tests { .global_vars .into_iter() .cloned() - .map(|mut v| { v.id = 0; v }) + .map(|mut v| { + v.id = 0; + v + }) .collect::>(), create_vars(&["var2", "var1"]) ); @@ -467,7 +502,9 @@ mod tests { ) .unwrap(); - let match_store = DefaultMatchStore::new(&[base_file.to_string_lossy().to_string()]); + let (match_store, non_fatal_error_sets) = + DefaultMatchStore::load(&[base_file.to_string_lossy().to_string()]); + assert_eq!(non_fatal_error_sets.len(), 0); let match_set = match_store.query(&[base_file.to_string_lossy().to_string()]); @@ -476,7 +513,10 @@ mod tests { .matches .into_iter() .cloned() - .map(|mut m| { m.id = 0; m }) + .map(|mut m| { + m.id = 0; + m + }) .collect::>(), create_matches(&[ ("hello", "world3"), @@ -491,7 +531,10 @@ mod tests { .global_vars .into_iter() .cloned() - .map(|mut v| { v.id = 0; v}) + .map(|mut v| { + v.id = 0; + v + }) .collect::>(), create_vars(&["var2", "var1"]) ); @@ -550,10 +593,11 @@ mod tests { ) .unwrap(); - let match_store = DefaultMatchStore::new(&[ + let (match_store, non_fatal_error_sets) = DefaultMatchStore::load(&[ base_file.to_string_lossy().to_string(), sub_file.to_string_lossy().to_string(), ]); + assert_eq!(non_fatal_error_sets.len(), 0); let match_set = match_store.query(&[ base_file.to_string_lossy().to_string(), @@ -565,7 +609,10 @@ mod tests { .matches .into_iter() .cloned() - .map(|mut m| { m.id = 0; m }) + .map(|mut m| { + m.id = 0; + m + }) .collect::>(), create_matches(&[ ("hello", "world2"), @@ -580,7 +627,10 @@ mod tests { .global_vars .into_iter() .cloned() - .map(|mut v| { v.id = 0; v }) + .map(|mut v| { + v.id = 0; + v + }) .collect::>(), create_vars(&["var1", "var2"]) ); @@ -642,7 +692,9 @@ mod tests { ) .unwrap(); - let match_store = DefaultMatchStore::new(&[base_file.to_string_lossy().to_string()]); + let (match_store, non_fatal_error_sets) = + DefaultMatchStore::load(&[base_file.to_string_lossy().to_string()]); + assert_eq!(non_fatal_error_sets.len(), 0); let match_set = match_store.query(&[ base_file.to_string_lossy().to_string(), @@ -654,7 +706,10 @@ mod tests { .matches .into_iter() .cloned() - .map(|mut m| { m.id = 0; m }) + .map(|mut m| { + m.id = 0; + m + }) .collect::>(), create_matches(&[ ("hello", "world3"), // This appears only once, though it appears 2 times @@ -669,10 +724,15 @@ mod tests { .global_vars .into_iter() .cloned() - .map(|mut v| { v.id = 0; v }) + .map(|mut v| { + v.id = 0; + v + }) .collect::>(), create_vars(&["var2", "var1"]) ); }); } + + // TODO: add fatal and non-fatal error cases } diff --git a/espanso-config/src/matches/store/mod.rs b/espanso-config/src/matches/store/mod.rs index 94580ea..2c252fa 100644 --- a/espanso-config/src/matches/store/mod.rs +++ b/espanso-config/src/matches/store/mod.rs @@ -17,12 +17,15 @@ * along with espanso. If not, see . */ +use crate::error::NonFatalErrorSet; + use super::{Match, Variable}; mod default; pub trait MatchStore: Send { fn query(&self, paths: &[String]) -> MatchSet; + fn loaded_paths(&self) -> Vec; } #[derive(Debug, Clone, PartialEq)] @@ -31,8 +34,8 @@ pub struct MatchSet<'a> { pub global_vars: Vec<&'a Variable>, } -pub fn new(paths: &[String]) -> impl MatchStore { +pub fn load(paths: &[String]) -> (impl MatchStore, Vec) { // TODO: here we can replace the DefaultMatchStore with a caching wrapper // that returns the same response for the given "paths" query - default::DefaultMatchStore::new(paths) + default::DefaultMatchStore::load(paths) }