From 3974d90bc92ca2447066fe734ccd0ce755c4ceda Mon Sep 17 00:00:00 2001 From: Federico Terzi Date: Fri, 5 Mar 2021 21:31:54 +0100 Subject: [PATCH] Refactor config structure and improve importer logic --- espanso-config/src/config/mod.rs | 2 +- espanso-config/src/config/path.rs | 16 +- .../src/config/{macro_util.rs => util.rs} | 0 espanso-config/src/config/yaml.rs | 5 +- .../src/matches/group/loader/mod.rs | 130 +++++++++++++++ .../group/{yaml.rs => loader/yaml/mod.rs} | 149 +++++------------- .../src/matches/group/loader/yaml/parse.rs | 101 ++++++++++++ espanso-config/src/matches/group/mod.rs | 112 +------------ espanso-config/src/matches/group/path.rs | 57 +++++++ espanso-config/src/matches/mod.rs | 16 +- espanso-config/src/matches/store/default.rs | 44 +++++- espanso-config/src/util.rs | 15 +- 12 files changed, 399 insertions(+), 248 deletions(-) rename espanso-config/src/config/{macro_util.rs => util.rs} (100%) create mode 100644 espanso-config/src/matches/group/loader/mod.rs rename espanso-config/src/matches/group/{yaml.rs => loader/yaml/mod.rs} (71%) create mode 100644 espanso-config/src/matches/group/loader/yaml/parse.rs create mode 100644 espanso-config/src/matches/group/path.rs diff --git a/espanso-config/src/config/mod.rs b/espanso-config/src/config/mod.rs index 50268ce..213a5b2 100644 --- a/espanso-config/src/config/mod.rs +++ b/espanso-config/src/config/mod.rs @@ -4,7 +4,7 @@ use anyhow::Result; mod yaml; mod path; -mod macro_util; +mod util; pub struct Config { pub label: Option, diff --git a/espanso-config/src/config/path.rs b/espanso-config/src/config/path.rs index 751cbf8..387a787 100644 --- a/espanso-config/src/config/path.rs +++ b/espanso-config/src/config/path.rs @@ -47,21 +47,9 @@ pub fn calculate_paths<'a>(base_dir: &Path, glob_patterns: impl Iterator bool; + fn load_group(&self, path: &Path) -> Result; +} + +lazy_static! { + static ref IMPORTERS: Vec> = vec![ + Box::new(YAMLImporter::new()), + ]; +} + +pub(crate) fn load_match_group(path: &Path) -> Result { + if let Some(extension) = path.extension() { + let extension = extension.to_string_lossy().to_lowercase(); + + let importer = IMPORTERS + .iter() + .find(|importer| importer.is_supported(&extension)); + + match importer { + Some(importer) => match importer.load_group(path) { + Ok(group) => Ok(group), + Err(err) => Err(LoadError::ParsingError(err).into()), + }, + None => Err(LoadError::InvalidFormat().into()), + } + } else { + Err(LoadError::MissingExtension().into()) + } +} + +#[derive(Error, Debug)] +pub enum LoadError { + #[error("missing extension in match group file")] + MissingExtension(), + + #[error("invalid match group format")] + InvalidFormat(), + + #[error("parser reported an error: `{0}`")] + ParsingError(anyhow::Error), +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::util::tests::use_test_directory; + + #[test] + fn load_group_invalid_format() { + use_test_directory(|_, match_dir, _| { + let file = match_dir.join("base.invalid"); + std::fs::write(&file, "test").unwrap(); + + assert!(matches!(load_match_group(&file).unwrap_err().downcast::().unwrap(), LoadError::InvalidFormat())); + }); + } + + #[test] + fn load_group_missing_extension() { + use_test_directory(|_, match_dir, _| { + let file = match_dir.join("base"); + std::fs::write(&file, "test").unwrap(); + + assert!(matches!(load_match_group(&file).unwrap_err().downcast::().unwrap(), LoadError::MissingExtension())); + }); + } + + #[test] + fn load_group_parsing_error() { + use_test_directory(|_, match_dir, _| { + let file = match_dir.join("base.yml"); + std::fs::write(&file, "test").unwrap(); + + assert!(matches!(load_match_group(&file).unwrap_err().downcast::().unwrap(), LoadError::ParsingError(_))); + }); + } + + #[test] + fn load_group_yaml_format() { + use_test_directory(|_, match_dir, _| { + let file = match_dir.join("base.yml"); + std::fs::write(&file, r#" + matches: + - trigger: "hello" + replace: "world" + "#).unwrap(); + + assert_eq!(load_match_group(&file).unwrap().matches.len(), 1); + }); + } + + #[test] + fn load_group_yaml_format_2() { + use_test_directory(|_, match_dir, _| { + let file = match_dir.join("base.yaml"); + std::fs::write(&file, r#" + matches: + - trigger: "hello" + replace: "world" + "#).unwrap(); + + assert_eq!(load_match_group(&file).unwrap().matches.len(), 1); + }); + } + + #[test] + fn load_group_yaml_format_casing() { + use_test_directory(|_, match_dir, _| { + let file = match_dir.join("base.YML"); + std::fs::write(&file, r#" + matches: + - trigger: "hello" + replace: "world" + "#).unwrap(); + + assert_eq!(load_match_group(&file).unwrap().matches.len(), 1); + }); + } +} diff --git a/espanso-config/src/matches/group/yaml.rs b/espanso-config/src/matches/group/loader/yaml/mod.rs similarity index 71% rename from espanso-config/src/matches/group/yaml.rs rename to espanso-config/src/matches/group/loader/yaml/mod.rs index b9e90fd..400376f 100644 --- a/espanso-config/src/matches/group/yaml.rs +++ b/espanso-config/src/matches/group/loader/yaml/mod.rs @@ -1,52 +1,39 @@ -use std::{collections::HashMap, convert::{TryFrom, TryInto}, path::Path}; - +use crate::matches::{Match, Variable, group::{MatchGroup, path::resolve_imports}}; +use log::warn; +use parse::YAMLMatchGroup; use anyhow::Result; -use serde::{Deserialize, Serialize}; -use serde_yaml::{Mapping, Value}; -use thiserror::Error; +use std::convert::{TryFrom, TryInto}; -use crate::util::is_yaml_empty; +use self::parse::{YAMLMatch, YAMLVariable}; +use crate::matches::{MatchCause, MatchEffect, TextEffect, TriggerCause}; -use crate::matches::{Match, MatchCause, MatchEffect, TextEffect, TriggerCause, Variable}; -use super::{MatchGroup}; +use super::Importer; -#[derive(Debug, Serialize, Deserialize, Clone)] -pub struct YAMLMatchGroup { - #[serde(default)] - pub imports: Option>, +mod parse; - #[serde(default)] - pub global_vars: Option>, +pub(crate) struct YAMLImporter {} - #[serde(default)] - pub matches: Option>, -} - -impl YAMLMatchGroup { - pub fn parse_from_str(yaml: &str) -> Result { - // Because an empty string is not valid YAML but we want to support it anyway - if is_yaml_empty(yaml) { - return Ok(serde_yaml::from_str( - "arbitrary_field_that_will_not_block_the_parser: true", - )?); - } - - Ok(serde_yaml::from_str(yaml)?) - } - - // TODO: test - pub fn parse_from_file(path: &Path) -> Result { - let content = std::fs::read_to_string(path)?; - Self::parse_from_str(&content) +impl YAMLImporter { + pub fn new() -> Self { + Self {} } } -impl TryFrom for MatchGroup { - type Error = anyhow::Error; +impl Importer for YAMLImporter { + fn is_supported(&self, extension: &str) -> bool { + extension == "yaml" || extension == "yml" + } // TODO: test - fn try_from(yaml_match_group: YAMLMatchGroup) -> Result { - let global_vars: Result> = yaml_match_group + // TODO: test resolve imports + // TODO: test cyclical dependency + fn load_group( + &self, + path: &std::path::Path, + ) -> anyhow::Result { + let yaml_group = YAMLMatchGroup::parse_from_file(path)?; + + let global_vars: Result> = yaml_group .global_vars .as_ref() .cloned() @@ -55,7 +42,7 @@ impl TryFrom for MatchGroup { .map(|var| var.clone().try_into()) .collect(); - let matches: Result> = yaml_match_group + let matches: Result> = yaml_group .matches .as_ref() .cloned() @@ -64,78 +51,17 @@ impl TryFrom for MatchGroup { .map(|m| m.clone().try_into()) .collect(); + // Resolve imports + let resolved_imports = resolve_imports(path, &yaml_group.imports.unwrap_or_default())?; + Ok(MatchGroup { - imports: yaml_match_group.imports.unwrap_or_default(), + imports: resolved_imports, global_vars: global_vars?, matches: matches?, - ..Default::default() }) } } -#[derive(Debug, Serialize, Deserialize, Clone)] -pub struct YAMLMatch { - #[serde(default)] - pub trigger: Option, - - #[serde(default)] - pub triggers: Option>, - - #[serde(default)] - pub replace: Option, - - #[serde(default)] - pub image_path: Option, // TODO: map - - #[serde(default)] - pub form: Option, // TODO: map - - #[serde(default)] - pub form_fields: Option>, // TODO: map - - #[serde(default)] - pub vars: Option>, - - #[serde(default)] - pub word: Option, - - #[serde(default)] - pub left_word: Option, - - #[serde(default)] - pub right_word: Option, - - #[serde(default)] - pub propagate_case: Option, - - #[serde(default)] - pub force_clipboard: Option, - - #[serde(default)] - pub markdown: Option, // TODO: map - - #[serde(default)] - pub paragraph: Option, // TODO: map - - #[serde(default)] - pub html: Option, // TODO: map -} - -#[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] -pub struct YAMLVariable { - pub name: String, - - #[serde(rename = "type")] - pub var_type: String, - - #[serde(default = "default_params")] - pub params: Mapping, -} - -fn default_params() -> Mapping { - Mapping::new() -} - impl TryFrom for Match { type Error = anyhow::Error; @@ -183,7 +109,9 @@ impl TryFrom for Match { MatchEffect::None }; - // TODO: log none match effect + if let MatchEffect::None = effect { + warn!("match caused by {:?} does not produce any effect. Did you forget the 'replace' field?", cause); + } Ok(Self { cause, @@ -208,16 +136,11 @@ impl TryFrom for Variable { } } -#[derive(Error, Debug)] -pub enum YAMLConversionError { - // TODO -//#[error("unknown data store error")] -//Unknown, -} - #[cfg(test)] mod tests { - use super::*; + use serde_yaml::{Mapping, Value}; + + use super::*; use crate::matches::Match; fn create_match(yaml: &str) -> Result { diff --git a/espanso-config/src/matches/group/loader/yaml/parse.rs b/espanso-config/src/matches/group/loader/yaml/parse.rs new file mode 100644 index 0000000..248fcfb --- /dev/null +++ b/espanso-config/src/matches/group/loader/yaml/parse.rs @@ -0,0 +1,101 @@ +use std::{collections::HashMap, path::Path}; + +use anyhow::Result; +use serde::{Deserialize, Serialize}; +use serde_yaml::{Mapping, Value}; + +use crate::util::is_yaml_empty; + +#[derive(Debug, Serialize, Deserialize, Clone)] +pub struct YAMLMatchGroup { + #[serde(default)] + pub imports: Option>, + + #[serde(default)] + pub global_vars: Option>, + + #[serde(default)] + pub matches: Option>, +} + +impl YAMLMatchGroup { + pub fn parse_from_str(yaml: &str) -> Result { + // Because an empty string is not valid YAML but we want to support it anyway + if is_yaml_empty(yaml) { + return Ok(serde_yaml::from_str( + "arbitrary_field_that_will_not_block_the_parser: true", + )?); + } + + Ok(serde_yaml::from_str(yaml)?) + } + + // TODO: test + pub fn parse_from_file(path: &Path) -> Result { + let content = std::fs::read_to_string(path)?; + Self::parse_from_str(&content) + } +} + +#[derive(Debug, Serialize, Deserialize, Clone)] +pub struct YAMLMatch { + #[serde(default)] + pub trigger: Option, + + #[serde(default)] + pub triggers: Option>, + + #[serde(default)] + pub replace: Option, + + #[serde(default)] + pub image_path: Option, // TODO: map + + #[serde(default)] + pub form: Option, // TODO: map + + #[serde(default)] + pub form_fields: Option>, // TODO: map + + #[serde(default)] + pub vars: Option>, + + #[serde(default)] + pub word: Option, + + #[serde(default)] + pub left_word: Option, + + #[serde(default)] + pub right_word: Option, + + #[serde(default)] + pub propagate_case: Option, + + #[serde(default)] + pub force_clipboard: Option, + + #[serde(default)] + pub markdown: Option, // TODO: map + + #[serde(default)] + pub paragraph: Option, // TODO: map + + #[serde(default)] + pub html: Option, // TODO: map +} + +#[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] +pub struct YAMLVariable { + pub name: String, + + #[serde(rename = "type")] + pub var_type: String, + + #[serde(default = "default_params")] + pub params: Mapping, +} + +fn default_params() -> Mapping { + Mapping::new() +} \ No newline at end of file diff --git a/espanso-config/src/matches/group/mod.rs b/espanso-config/src/matches/group/mod.rs index bd87c77..05be22d 100644 --- a/espanso-config/src/matches/group/mod.rs +++ b/espanso-config/src/matches/group/mod.rs @@ -1,22 +1,18 @@ use anyhow::Result; -use log::error; use std::{ - cell::RefCell, - convert::TryInto, - path::{Path, PathBuf}, + path::{Path}, }; -use thiserror::Error; use super::{Match, Variable}; -mod yaml; +mod loader; +mod path; + #[derive(Debug, Clone, PartialEq)] pub(crate) struct MatchGroup { - imports: Vec, + pub imports: Vec, pub global_vars: Vec, pub matches: Vec, - - pub resolved_imports: Vec, } impl Default for MatchGroup { @@ -25,7 +21,6 @@ impl Default for MatchGroup { imports: Vec::new(), global_vars: Vec::new(), matches: Vec::new(), - resolved_imports: Vec::new(), } } } @@ -33,101 +28,6 @@ impl Default for MatchGroup { impl MatchGroup { // TODO: test pub fn load(group_path: &Path) -> Result { - if let Some(extension) = group_path.extension() { - let extension = extension.to_string_lossy().to_lowercase(); - - if extension == "yml" || extension == "yaml" { - match yaml::YAMLMatchGroup::parse_from_file(group_path) { - Ok(yaml_group) => { - let match_group: Result = yaml_group.try_into(); - match match_group { - Ok(mut group) => { - group.resolve_imports(group_path)?; - Ok(group) - } - Err(err) => Err(MatchGroupError::ParsingError(err).into()), - } - } - Err(err) => Err(MatchGroupError::ParsingError(err).into()), - } - } else { - Err(MatchGroupError::InvalidFormat().into()) - } - } else { - Err(MatchGroupError::MissingExtension().into()) - } - } - - // TODO: test - fn resolve_imports(&mut self, group_path: &Path) -> Result<()> { - let mut paths = Vec::new(); - - if !group_path.exists() { - return Err( - MatchGroupError::ResolveImportFailed(format!( - "unable to resolve imports for match group at path: {:?}", - group_path - )) - .into(), - ); - } - - // Get the containing directory - let current_dir = if group_path.is_file() { - if let Some(parent) = group_path.parent() { - parent - } else { - return Err( - MatchGroupError::ResolveImportFailed(format!( - "unable to resolve imports for match group starting from current path: {:?}", - group_path - )) - .into(), - ); - } - } else { - group_path - }; - - for import in self.imports.iter() { - let import_path = PathBuf::from(import); - - // Absolute or relative import - let full_path = if import_path.is_relative() { - current_dir.join(import_path) - } else { - import_path - }; - - if full_path.exists() && full_path.is_file() { - paths.push(full_path) - } else { - // Best effort imports - error!("unable to resolve import at path: {:?}", full_path); - } - } - - let string_paths = paths - .into_iter() - .map(|path| path.to_string_lossy().to_string()) - .collect(); - self.resolved_imports = string_paths; - - Ok(()) + loader::load_match_group(group_path) } } - -#[derive(Error, Debug)] -pub enum MatchGroupError { - #[error("missing extension in match group file")] - MissingExtension(), - - #[error("invalid match group format")] - InvalidFormat(), - - #[error("parser reported an error: `{0}`")] - ParsingError(anyhow::Error), - - #[error("resolve import failed: `{0}`")] - ResolveImportFailed(String), -} diff --git a/espanso-config/src/matches/group/path.rs b/espanso-config/src/matches/group/path.rs new file mode 100644 index 0000000..ba1773c --- /dev/null +++ b/espanso-config/src/matches/group/path.rs @@ -0,0 +1,57 @@ +use std::path::{Path, PathBuf}; +use anyhow::Result; +use log::error; +use thiserror::Error; + +// TODO: test +pub fn resolve_imports(group_path: &Path, imports: &[String]) -> Result> { + let mut paths = Vec::new(); + + // Get the containing directory + let current_dir = if group_path.is_file() { + if let Some(parent) = group_path.parent() { + parent + } else { + return Err( + ResolveImportError::Failed(format!( + "unable to resolve imports for match group starting from current path: {:?}", + group_path + )) + .into(), + ); + } + } else { + group_path + }; + + for import in imports.iter() { + let import_path = PathBuf::from(import); + + // Absolute or relative import + let full_path = if import_path.is_relative() { + current_dir.join(import_path) + } else { + import_path + }; + + if full_path.exists() && full_path.is_file() { + paths.push(full_path) + } else { + // Best effort imports + error!("unable to resolve import at path: {:?}", full_path); + } + } + + let string_paths = paths + .into_iter() + .map(|path| path.to_string_lossy().to_string()) + .collect(); + + Ok(string_paths) +} + +#[derive(Error, Debug)] +pub enum ResolveImportError { + #[error("resolve import failed: `{0}`")] + Failed(String), +} \ No newline at end of file diff --git a/espanso-config/src/matches/mod.rs b/espanso-config/src/matches/mod.rs index 30d0a69..23cd181 100644 --- a/espanso-config/src/matches/mod.rs +++ b/espanso-config/src/matches/mod.rs @@ -5,7 +5,7 @@ use crate::counter::{next_id, StructId}; mod group; mod store; -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone)] pub struct Match { cause: MatchCause, effect: MatchEffect, @@ -28,6 +28,12 @@ impl Default for Match { } } +impl PartialEq for Match { + fn eq(&self, other: &Self) -> bool { + self.cause == other.cause && self.effect == other.effect && self.label == other.label + } +} + // Causes #[derive(Debug, Clone, PartialEq)] @@ -84,7 +90,7 @@ impl Default for TextEffect { } } -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone)] pub struct Variable { pub name: String, pub var_type: String, @@ -104,3 +110,9 @@ impl Default for Variable { } } } + +impl PartialEq for Variable { + fn eq(&self, other: &Self) -> bool { + self.name == other.name && self.var_type == other.var_type && self.params == other.params + } +} diff --git a/espanso-config/src/matches/store/default.rs b/espanso-config/src/matches/store/default.rs index 689b572..c3e9140 100644 --- a/espanso-config/src/matches/store/default.rs +++ b/espanso-config/src/matches/store/default.rs @@ -4,8 +4,11 @@ use std::{ path::PathBuf, }; -use super::MatchStore; -use crate::{counter::StructId, matches::{group::MatchGroup, Match, Variable}}; +use super::{MatchSet, MatchStore}; +use crate::{ + counter::StructId, + matches::{group::MatchGroup, Match, Variable}, +}; // TODO: implement store according to notes pub(crate) struct DefaultMatchStore { @@ -32,11 +35,27 @@ impl MatchStore for DefaultMatchStore { // TODO: test // TODO: test for cyclical imports - fn query_set(&self, paths: &[String]) -> super::MatchSet { + fn query_set(&self, paths: &[String]) -> MatchSet { let mut matches: Vec<&Match> = Vec::new(); let mut global_vars: Vec<&Variable> = Vec::new(); + let mut visited_paths = HashSet::new(); + let mut visited_matches = HashSet::new(); + let mut visited_global_vars = HashSet::new(); - todo!() + query_matches_for_paths( + &self.groups, + &mut visited_paths, + &mut visited_matches, + &mut visited_global_vars, + &mut matches, + &mut global_vars, + paths, + ); + + MatchSet { + matches, + global_vars, + } } } @@ -46,9 +65,9 @@ fn load_match_groups_recursively(groups: &mut HashMap, paths let group_path = PathBuf::from(path); match MatchGroup::load(&group_path) { Ok(group) => { - load_match_groups_recursively(groups, &group.resolved_imports); + load_match_groups_recursively(groups, &group.imports); groups.insert(path.clone(), group); - }, + } Err(error) => { error!("unable to load match group: {:?}", error); } @@ -57,8 +76,9 @@ fn load_match_groups_recursively(groups: &mut HashMap, paths } } +// TODO: test fn query_matches_for_paths<'a>( - groups: &'a mut HashMap, + groups: &'a HashMap, visited_paths: &mut HashSet, visited_matches: &mut HashSet, visited_global_vars: &mut HashSet, @@ -83,7 +103,15 @@ fn query_matches_for_paths<'a>( } } - // TODO: here we should visit the imported paths recursively + query_matches_for_paths( + groups, + visited_paths, + visited_matches, + visited_global_vars, + matches, + global_vars, + &group.imports, + ) } visited_paths.insert(path.clone()); diff --git a/espanso-config/src/util.rs b/espanso-config/src/util.rs index d625b75..1ef0aa3 100644 --- a/espanso-config/src/util.rs +++ b/espanso-config/src/util.rs @@ -13,8 +13,21 @@ pub fn is_yaml_empty(yaml: &str) -> bool { } #[cfg(test)] -mod tests { +pub mod tests { use super::*; + use std::{fs::create_dir_all, path::Path}; + use tempdir::TempDir; + + pub fn use_test_directory(callback: impl FnOnce(&Path, &Path, &Path)) { + let dir = TempDir::new("tempconfig").unwrap(); + let match_dir = dir.path().join("match"); + create_dir_all(&match_dir).unwrap(); + + let config_dir = dir.path().join("config"); + create_dir_all(&config_dir).unwrap(); + + callback(&dir.path(), &match_dir, &config_dir); + } #[test] fn is_yaml_empty_document_empty() {