From eca7219039c663d2ae03ad53012c8e1352433982 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sat, 28 Oct 2017 12:50:22 +0200 Subject: [PATCH 1/2] Rewrite configuration providing in runtime Before the configuration object (the raw TOML object) was provided via a wrapper object `Configuration`. This was ugly and not very nice to use. Now, we only have the `toml::Value` object we lend out from `Runtime::config()`. The changes included libimagrt internal rewrites, which are not visible to the user. Anyways, this change changes the API for config-fetching from the runtime, so fixes for all other crates may follow. The changes also removed the support for reading the "editor" setting from the configuration file, which was not used anyways (in the example imagrc.toml file). The CLI-reading and ENV-reading are still supported, though. --- doc/src/09020-changelog.md | 3 + lib/core/libimagrt/src/configuration.rs | 293 +++++++----------------- lib/core/libimagrt/src/error.rs | 25 ++ lib/core/libimagrt/src/logger.rs | 19 +- lib/core/libimagrt/src/runtime.rs | 28 +-- 5 files changed, 126 insertions(+), 242 deletions(-) diff --git a/doc/src/09020-changelog.md b/doc/src/09020-changelog.md index 763c4763..baa9e0d6 100644 --- a/doc/src/09020-changelog.md +++ b/doc/src/09020-changelog.md @@ -30,6 +30,9 @@ This section contains the changelog from the last release to the next release. * `imag-store` can dump all storeids now * `imag-annotate` was introduced * `imag-diagnostics` was added + * The runtime does not read the config file for editor settings anymore. + Specifying an editor either via CLI or via the `$EDITOR` environment + variable still possible. * Minor changes * `libimagentryannotation` got a rewrite, is not based on `libimagnotes` diff --git a/lib/core/libimagrt/src/configuration.rs b/lib/core/libimagrt/src/configuration.rs index 985c9446..88ed6512 100644 --- a/lib/core/libimagrt/src/configuration.rs +++ b/lib/core/libimagrt/src/configuration.rs @@ -18,228 +18,23 @@ // use std::path::PathBuf; -use std::ops::Deref; use toml::Value; use clap::App; -error_chain! { - types { - ConfigError, ConfigErrorKind, ResultExt, Result; - } +use error::RuntimeError as RE; +use error::RuntimeErrorKind as REK; +use error::Result; +use error::ResultExt; - errors { - TOMLParserError { - description("TOML Parsing error") - display("TOML Parsing error") - } - - NoConfigFileFound { - description("No config file found") - display("No config file found") - } - - ConfigOverrideError { - description("Config override error") - display("Config override error") - } - - ConfigOverrideKeyNotAvailable { - description("Key not available") - display("Key not available") - } - - ConfigOverrideTypeNotMatching { - description("Configuration Type not matching") - display("Configuration Type not matching") - } - } -} -use self::ConfigErrorKind as CEK; -use self::ConfigError as CE; - -/// `Configuration` object +/// Get a new configuration object. /// -/// Holds all config variables which are globally available plus the configuration object from the -/// config parser, which can be accessed. -#[derive(Debug)] -pub struct Configuration { - - /// The plain configuration object for direct access if necessary - config: Value, - - /// The verbosity the program should run with - verbosity: bool, - - /// The editor which should be used - editor: Option, - - ///The options the editor should get when opening some file - editor_opts: String, -} - -impl Configuration { - - /// Get a new configuration object. - /// - /// The passed runtimepath is used for searching the configuration file, whereas several file - /// names are tested. If that does not work, the home directory and the XDG basedir are tested - /// with all variants. - /// - /// If that doesn't work either, an error is returned. - pub fn new(config_searchpath: &PathBuf) -> Result { - fetch_config(&config_searchpath).map(|cfg| { - let verbosity = get_verbosity(&cfg); - let editor = get_editor(&cfg); - let editor_opts = get_editor_opts(&cfg); - - debug!("Building configuration"); - debug!(" - verbosity : {:?}", verbosity); - debug!(" - editor : {:?}", editor); - debug!(" - editor-opts: {}", editor_opts); - - Configuration { - config: cfg, - verbosity: verbosity, - editor: editor, - editor_opts: editor_opts, - } - }) - } - - /// Get a new configuration object built from the given toml value. - pub fn with_value(value: Value) -> Configuration { - Configuration{ - verbosity: get_verbosity(&value), - editor: get_editor(&value), - editor_opts: get_editor_opts(&value), - config: value, - } - } - - /// Get the Editor setting from the configuration - pub fn editor(&self) -> Option<&String> { - self.editor.as_ref() - } - - /// Get the underlying configuration TOML object - pub fn config(&self) -> &Value { - &self.config - } - - /// Get the configuration of the store, if any. - pub fn store_config(&self) -> Option<&Value> { - match self.config { - Value::Table(ref tabl) => tabl.get("store"), - _ => None, - } - } - - /// Override the configuration. - /// The `v` parameter is expected to contain 'key=value' pairs where the key is a path in the - /// TOML tree, the value to be an appropriate value. - /// - /// The override fails if the configuration which is about to be overridden does not exist or - /// the `value` part cannot be converted to the type of the configuration value. - /// - /// If `v` is empty, this is considered to be a successful `override_config()` call. - pub fn override_config(&mut self, v: Vec) -> Result<()> { - use libimagutil::key_value_split::*; - use toml_query::read::TomlValueReadExt; - - let iter = v.into_iter() - .map(|s| { debug!("Trying to process '{}'", s); s }) - .filter_map(|s| match s.into_kv() { - Some(kv) => Some(kv.into()), - None => { - warn!("Could split at '=' - will be ignore override"); - None - } - }) - .map(|(k, v)| self - .config - .read(&k[..]) - .chain_err(|| CEK::TOMLParserError) - .map(|toml| match toml { - Some(value) => match into_value(value, v) { - Some(v) => { - info!("Successfully overridden: {} = {}", k, v); - Ok(()) - }, - None => Err(CE::from_kind(CEK::ConfigOverrideTypeNotMatching)), - }, - None => Err(CE::from_kind(CEK::ConfigOverrideKeyNotAvailable)), - }) - ); - - for elem in iter { - let _ = try!(elem.chain_err(|| CEK::ConfigOverrideError)); - } - - Ok(()) - } -} - -/// Tries to convert the String `s` into the same type as `value`. +/// The passed runtimepath is used for searching the configuration file, whereas several file +/// names are tested. If that does not work, the home directory and the XDG basedir are tested +/// with all variants. /// -/// Returns None if string cannot be converted. -/// -/// Arrays and Tables are not supported and will yield `None`. -fn into_value(value: &Value, s: String) -> Option { - use std::str::FromStr; - - match *value { - Value::String(_) => Some(Value::String(s)), - Value::Integer(_) => FromStr::from_str(&s[..]).ok().map(Value::Integer), - Value::Float(_) => FromStr::from_str(&s[..]).ok().map(Value::Float), - Value::Boolean(_) => { - if s == "true" { Some(Value::Boolean(true)) } - else if s == "false" { Some(Value::Boolean(false)) } - else { None } - } - Value::Datetime(_) => Value::try_from(s).ok(), - _ => None, - } -} - -impl Deref for Configuration { - type Target = Value; - - fn deref(&self) -> &Value { - &self.config - } - -} - -fn get_verbosity(v: &Value) -> bool { - match *v { - Value::Table(ref t) => t.get("verbose") - .map_or(false, |v| is_match!(v, &Value::Boolean(true))), - _ => false, - } -} - -fn get_editor(v: &Value) -> Option { - match *v { - Value::Table(ref t) => t.get("editor") - .and_then(|v| match *v { Value::String(ref s) => Some(s.clone()), _ => None, }), - _ => None, - } -} - -fn get_editor_opts(v: &Value) -> String { - match *v { - Value::Table(ref t) => t.get("editor-opts") - .and_then(|v| match *v { Value::String(ref s) => Some(s.clone()), _ => None, }) - .unwrap_or_default(), - _ => String::new(), - } -} - -/// Helper to fetch the config file -/// -/// Tests several variants for the config file path and uses the first one which works. -fn fetch_config(searchpath: &PathBuf) -> Result { +/// If that doesn't work either, an error is returned. +pub fn fetch_config(searchpath: &PathBuf) -> Result { use std::env; use std::fs::File; use std::io::Read; @@ -302,7 +97,72 @@ fn fetch_config(searchpath: &PathBuf) -> Result { } }) .nth(0) - .ok_or(CE::from_kind(ConfigErrorKind::NoConfigFileFound)) + .ok_or(RE::from_kind(REK::ConfigNoConfigFileFound)) +} + +/// Override the configuration. +/// The `v` parameter is expected to contain 'key=value' pairs where the key is a path in the +/// TOML tree, the value to be an appropriate value. +/// +/// The override fails if the configuration which is about to be overridden does not exist or +/// the `value` part cannot be converted to the type of the configuration value. +/// +/// If `v` is empty, this is considered to be a successful `override_config()` call. +pub fn override_config(val: &mut Value, v: Vec) -> Result<()> { + use libimagutil::key_value_split::*; + use toml_query::read::TomlValueReadExt; + + let iter = v.into_iter() + .map(|s| { debug!("Trying to process '{}'", s); s }) + .filter_map(|s| match s.into_kv() { + Some(kv) => Some(kv.into()), + None => { + warn!("Could split at '=' - will be ignore override"); + None + } + }) + .map(|(k, v)| val + .read(&k[..]) + .chain_err(|| REK::ConfigTOMLParserError) + .map(|toml| match toml { + Some(value) => match into_value(value, v) { + Some(v) => { + info!("Successfully overridden: {} = {}", k, v); + Ok(()) + }, + None => Err(RE::from_kind(REK::ConfigOverrideTypeNotMatching)), + }, + None => Err(RE::from_kind(REK::ConfigOverrideKeyNotAvailable)), + }) + ); + + for elem in iter { + let _ = try!(elem.chain_err(|| REK::ConfigOverrideError)); + } + + Ok(()) +} + +/// Tries to convert the String `s` into the same type as `value`. +/// +/// Returns None if string cannot be converted. +/// +/// Arrays and Tables are not supported and will yield `None`. +fn into_value(value: &Value, s: String) -> Option { + use std::str::FromStr; + + match *value { + Value::String(_) => Some(Value::String(s)), + Value::Integer(_) => FromStr::from_str(&s[..]).ok().map(Value::Integer), + Value::Float(_) => FromStr::from_str(&s[..]).ok().map(Value::Float), + Value::Boolean(_) => { + if s == "true" { Some(Value::Boolean(true)) } + else if s == "false" { Some(Value::Boolean(false)) } + else { None } + } + Value::Datetime(_) => Value::try_from(s).ok(), + _ => None, + } } pub trait InternalConfiguration { @@ -316,3 +176,4 @@ pub trait InternalConfiguration { } impl<'a> InternalConfiguration for App<'a, 'a> {} + diff --git a/lib/core/libimagrt/src/error.rs b/lib/core/libimagrt/src/error.rs index a0e32d1b..4537c8a6 100644 --- a/lib/core/libimagrt/src/error.rs +++ b/lib/core/libimagrt/src/error.rs @@ -93,6 +93,31 @@ error_chain! { display("Missing config for logging format for error logging") } + ConfigTOMLParserError { + description("Configuration: TOML Parsing error") + display("Configuration: TOML Parsing error") + } + + ConfigNoConfigFileFound { + description("Configuration: No config file found") + display("Configuration: No config file found") + } + + ConfigOverrideError { + description("Configuration: Config override error") + display("Configuration: Config override error") + } + + ConfigOverrideKeyNotAvailable { + description("Configuration: Key not available") + display("Configuration: Key not available") + } + + ConfigOverrideTypeNotMatching { + description("Configuration: Configuration Type not matching") + display("Configuration: Configuration Type not matching") + } + } } diff --git a/lib/core/libimagrt/src/logger.rs b/lib/core/libimagrt/src/logger.rs index 241ff73c..60f50d6d 100644 --- a/lib/core/libimagrt/src/logger.rs +++ b/lib/core/libimagrt/src/logger.rs @@ -24,7 +24,6 @@ use std::sync::Arc; use std::sync::Mutex; use std::ops::Deref; -use configuration::Configuration; use error::RuntimeErrorKind as EK; use error::RuntimeError as RE; use error::ResultExt; @@ -77,7 +76,7 @@ pub struct ImagLogger { impl ImagLogger { /// Create a new ImagLogger object with a certain level - pub fn new(matches: &ArgMatches, config: Option<&Configuration>) -> Result { + pub fn new(matches: &ArgMatches, config: Option<&Value>) -> Result { let mut handlebars = Handlebars::new(); handlebars.register_escape_fn(::handlebars::no_escape); @@ -220,7 +219,7 @@ fn match_log_level_str(s: &str) -> Result { } } -fn aggregate_global_loglevel(matches: &ArgMatches, config: Option<&Configuration>) +fn aggregate_global_loglevel(matches: &ArgMatches, config: Option<&Value>) -> Result { fn get_arg_loglevel(matches: &ArgMatches) -> Result> { @@ -300,7 +299,7 @@ fn translate_destinations(raw: &Vec) -> Result> { }) } -fn aggregate_global_destinations(matches: &ArgMatches, config: Option<&Configuration>) +fn aggregate_global_destinations(matches: &ArgMatches, config: Option<&Value>) -> Result> { @@ -344,7 +343,7 @@ macro_rules! aggregate_global_format { }; } -fn aggregate_global_format_trace(config: Option<&Configuration>) +fn aggregate_global_format_trace(config: Option<&Value>) -> Result { aggregate_global_format!("imag.logging.format.trace", @@ -352,7 +351,7 @@ fn aggregate_global_format_trace(config: Option<&Configuration>) config) } -fn aggregate_global_format_debug(config: Option<&Configuration>) +fn aggregate_global_format_debug(config: Option<&Value>) -> Result { aggregate_global_format!("imag.logging.format.debug", @@ -360,7 +359,7 @@ fn aggregate_global_format_debug(config: Option<&Configuration>) config) } -fn aggregate_global_format_info(config: Option<&Configuration>) +fn aggregate_global_format_info(config: Option<&Value>) -> Result { aggregate_global_format!("imag.logging.format.info", @@ -368,7 +367,7 @@ fn aggregate_global_format_info(config: Option<&Configuration>) config) } -fn aggregate_global_format_warn(config: Option<&Configuration>) +fn aggregate_global_format_warn(config: Option<&Value>) -> Result { aggregate_global_format!("imag.logging.format.warn", @@ -376,7 +375,7 @@ fn aggregate_global_format_warn(config: Option<&Configuration>) config) } -fn aggregate_global_format_error(config: Option<&Configuration>) +fn aggregate_global_format_error(config: Option<&Value>) -> Result { aggregate_global_format!("imag.logging.format.error", @@ -384,7 +383,7 @@ fn aggregate_global_format_error(config: Option<&Configuration>) config) } -fn aggregate_module_settings(_matches: &ArgMatches, config: Option<&Configuration>) +fn aggregate_module_settings(_matches: &ArgMatches, config: Option<&Value>) -> Result> { match config { diff --git a/lib/core/libimagrt/src/runtime.rs b/lib/core/libimagrt/src/runtime.rs index 985ff990..8e805723 100644 --- a/lib/core/libimagrt/src/runtime.rs +++ b/lib/core/libimagrt/src/runtime.rs @@ -23,11 +23,13 @@ use std::env; use std::process::exit; pub use clap::App; +use toml::Value; +use toml_query::read::TomlValueReadExt; use clap::{Arg, ArgMatches}; use log; -use configuration::{Configuration, InternalConfiguration}; +use configuration::{fetch_config, override_config, InternalConfiguration}; use error::RuntimeError; use error::RuntimeErrorKind; use error::ResultExt; @@ -44,7 +46,7 @@ use spec::CliSpec; #[derive(Debug)] pub struct Runtime<'a> { rtp: PathBuf, - configuration: Option, + configuration: Option, cli_matches: ArgMatches<'a>, store: Store, } @@ -61,8 +63,6 @@ impl<'a> Runtime<'a> { { use libimagerror::trace::trace_error; - use configuration::ConfigErrorKind; - let matches = cli_app.clone().matches(); let rtp = get_rtp_match(&matches); @@ -72,8 +72,8 @@ impl<'a> Runtime<'a> { debug!("Config path = {:?}", configpath); - let config = match Configuration::new(&configpath) { - Err(e) => if !is_match!(e.kind(), &ConfigErrorKind::NoConfigFileFound) { + let config = match fetch_config(&configpath) { + Err(e) => if !is_match!(e.kind(), &RuntimeErrorKind::ConfigNoConfigFileFound) { return Err(e).chain_err(|| RuntimeErrorKind::Instantiate); } else { println!("No config file found."); @@ -82,7 +82,7 @@ impl<'a> Runtime<'a> { }, Ok(mut config) => { - if let Err(e) = config.override_config(get_override_specs(&matches)) { + if let Err(e) = override_config(&mut config, get_override_specs(&matches)) { error!("Could not apply config overrides"); trace_error(&e); @@ -97,7 +97,7 @@ impl<'a> Runtime<'a> { } /// Builds the Runtime object using the given `config`. - pub fn with_configuration(cli_app: C, config: Option) + pub fn with_configuration(cli_app: C, config: Option) -> Result, RuntimeError> where C: Clone + CliSpec<'a> + InternalConfiguration { @@ -105,7 +105,7 @@ impl<'a> Runtime<'a> { Runtime::_new(cli_app, matches, config) } - fn _new(mut cli_app: C, matches: ArgMatches<'a>, config: Option) + fn _new(mut cli_app: C, matches: ArgMatches<'a>, config: Option) -> Result, RuntimeError> where C: Clone + CliSpec<'a> + InternalConfiguration { @@ -139,7 +139,7 @@ impl<'a> Runtime<'a> { debug!("Store path = {:?}", storepath); let store_config = match config { - Some(ref c) => c.store_config().cloned(), + Some(ref c) => c.read("store").chain_err(|| RuntimeErrorKind::Instantiate)?.cloned(), None => None, }; @@ -345,7 +345,7 @@ impl<'a> Runtime<'a> { } /// Initialize the internal logger - fn init_logger(matches: &ArgMatches, config: Option<&Configuration>) { + fn init_logger(matches: &ArgMatches, config: Option<&Value>) { use std::env::var as env_var; use env_logger; @@ -386,7 +386,7 @@ impl<'a> Runtime<'a> { } /// Get the configuration object - pub fn config(&self) -> Option<&Configuration> { + pub fn config(&self) -> Option<&Value> { self.configuration.as_ref() } @@ -444,10 +444,6 @@ impl<'a> Runtime<'a> { self.cli() .value_of("editor") .map(String::from) - .or(match self.configuration { - Some(ref c) => c.editor().cloned(), - _ => None, - }) .or(env::var("EDITOR").ok()) .map(Command::new) } From b237adfe191f5440081d6297dc48fe58096cc23e Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sat, 28 Oct 2017 19:19:46 +0200 Subject: [PATCH 2/2] Fix calls to Runtime::config() to use direct result --- bin/core/imag-view/src/main.rs | 2 +- bin/core/imag/src/main.rs | 1 - bin/domain/imag-bookmark/src/main.rs | 2 +- bin/domain/imag-diary/src/util.rs | 1 - lib/domain/libimagdiary/src/config.rs | 10 ++++------ lib/etc/libimaginteraction/src/readline.rs | 3 +-- lib/etc/libimagutil/src/testing.rs | 8 ++++---- 7 files changed, 11 insertions(+), 16 deletions(-) diff --git a/bin/core/imag-view/src/main.rs b/bin/core/imag-view/src/main.rs index 09f88f83..ec13e0fe 100644 --- a/bin/core/imag-view/src/main.rs +++ b/bin/core/imag-view/src/main.rs @@ -99,7 +99,7 @@ fn main() { .map_err_trace_exit_unwrap(1); let query = format!("view.viewers.{}", viewer); - match config.config().read(&query) { + match config.read(&query) { Err(e) => trace_error_exit(&e, 1), Ok(None) => { error!("Cannot find '{}' in config", query); diff --git a/bin/core/imag/src/main.rs b/bin/core/imag/src/main.rs index 8d7da252..0b75574d 100644 --- a/bin/core/imag/src/main.rs +++ b/bin/core/imag/src/main.rs @@ -256,7 +256,6 @@ fn main() { fn fetch_aliases(rt: &Runtime) -> Result, String> { let cfg = try!(rt.config().ok_or_else(|| String::from("No configuration found"))); let value = cfg - .config() .read("imag.aliases") .map_err(|_| String::from("Reading from config failed")); diff --git a/bin/domain/imag-bookmark/src/main.rs b/bin/domain/imag-bookmark/src/main.rs index c4401dbf..7f2131ce 100644 --- a/bin/domain/imag-bookmark/src/main.rs +++ b/bin/domain/imag-bookmark/src/main.rs @@ -169,7 +169,7 @@ fn get_collection_name(rt: &Runtime, .and_then(|scmd| scmd.value_of(collection_argument_name).map(String::from)) .unwrap_or_else(|| { rt.config() - .map(|cfg| match cfg.config().read("bookmark.default_collection") { + .map(|cfg| match cfg.read("bookmark.default_collection") { Err(e) => trace_error_exit(&e, 1), Ok(Some(&Value::String(ref name))) => name.clone(), Ok(None) => { diff --git a/bin/domain/imag-diary/src/util.rs b/bin/domain/imag-diary/src/util.rs index a6ed9342..40811c60 100644 --- a/bin/domain/imag-diary/src/util.rs +++ b/bin/domain/imag-diary/src/util.rs @@ -53,7 +53,6 @@ pub fn get_diary_timed_config(rt: &Runtime, diary_name: &str) -> Result Ok(None), Some(cfg) => { let v = cfg - .config() .read(&format!("diary.diaries.{}.timed", diary_name)) .chain_err(|| DiaryErrorKind::IOError); diff --git a/lib/domain/libimagdiary/src/config.rs b/lib/domain/libimagdiary/src/config.rs index c2a6a980..e306e5d7 100644 --- a/lib/domain/libimagdiary/src/config.rs +++ b/lib/domain/libimagdiary/src/config.rs @@ -34,10 +34,8 @@ pub fn get_default_diary_name(rt: &Runtime) -> Option { } pub fn get_diary_config_section<'a>(rt: &'a Runtime) -> Option<&'a Value> { - rt.config() - .map(|config| config.config()) - .and_then(|config| match config.read(&String::from("diary")) { - Ok(x) => x, - Err(_) => None, - }) + rt.config().and_then(|config| match config.read(&String::from("diary")) { + Ok(x) => x, + Err(_) => None, + }) } diff --git a/lib/etc/libimaginteraction/src/readline.rs b/lib/etc/libimaginteraction/src/readline.rs index 7d498ce5..3f9a6399 100644 --- a/lib/etc/libimaginteraction/src/readline.rs +++ b/lib/etc/libimaginteraction/src/readline.rs @@ -34,9 +34,8 @@ pub struct Readline { impl Readline { pub fn new(rt: &Runtime) -> Result { - let cfg = try!(rt.config().ok_or(IEK::NoConfigError)); + let c = try!(rt.config().ok_or(IEK::NoConfigError)); - let c = cfg.config(); let histfile = try!(c.lookup("ui.cli.readline_history_file").ok_or(IEK::ConfigError)); let histsize = try!(c.lookup("ui.cli.readline_history_size").ok_or(IEK::ConfigError)); let histigndups = try!(c.lookup("ui.cli.readline_history_ignore_dups").ok_or(IEK::ConfigError)); diff --git a/lib/etc/libimagutil/src/testing.rs b/lib/etc/libimagutil/src/testing.rs index 183afb63..168a2650 100644 --- a/lib/etc/libimagutil/src/testing.rs +++ b/lib/etc/libimagutil/src/testing.rs @@ -51,7 +51,8 @@ macro_rules! make_mock_app { use libimagrt::spec::CliSpec; use libimagrt::runtime::Runtime; use libimagrt::error::RuntimeError; - use libimagrt::configuration::{Configuration, InternalConfiguration}; + use libimagrt::configuration::InternalConfiguration; + use toml::Value; #[derive(Clone)] struct MockLinkApp<'a> { @@ -89,9 +90,8 @@ macro_rules! make_mock_app { } #[allow(unused)] - pub fn generate_minimal_test_config() -> Option { ::toml::de::from_str("[store]\nimplicit-create=true") - .map(Configuration::with_value) - .ok() + pub fn generate_minimal_test_config() -> Option { + ::toml::de::from_str("[store]\nimplicit-create=true").ok() } #[allow(unused)]