From ea84f4f0310a95e2ea9b3ab1fc2c5586a7a25abe Mon Sep 17 00:00:00 2001 From: Dorota Czaplejewicz Date: Thu, 16 Jan 2020 15:57:46 +0000 Subject: [PATCH] logging: Try to improve common operations This adds sugar for logging `Result`s with a handler, makes names evoke something closer to "logging" than "warning", tries to remove any redundant `Logging` where the module name will do, and introduces a type strictly for bad things happening. --- src/data.rs | 106 ++++++++++++++++++++++---------------------- src/logging.rs | 117 +++++++++++++++++++++++++++++++++++++++---------- src/popover.rs | 8 +++- src/style.rs | 7 ++- src/tests.rs | 19 +++++--- 5 files changed, 168 insertions(+), 89 deletions(-) diff --git a/src/data.rs b/src/data.rs index 0fa30751..bd32ed88 100644 --- a/src/data.rs +++ b/src/data.rs @@ -21,7 +21,7 @@ use ::keyboard::{ }; use ::layout; use ::layout::ArrangementKind; -use ::logging::PrintWarnings; +use ::logging; use ::resources; use ::util::c::as_str; use ::util::hash_map_map; @@ -31,7 +31,7 @@ use ::xdg; use serde::Deserialize; use std::io::BufReader; use std::iter::FromIterator; -use ::logging::WarningHandler; +use ::logging::Warn; /// Gathers stuff defined in C or called by C pub mod c { @@ -157,7 +157,7 @@ fn list_layout_sources( fn load_layout_data(source: DataSource) -> Result<::layout::LayoutData, LoadError> { - let handler = PrintWarnings{}; + let handler = logging::Print {}; match source { DataSource::File(path) => { Layout::from_file(path.clone()) @@ -330,7 +330,7 @@ impl Layout { serde_yaml::from_reader(infile).map_err(Error::Yaml) } - pub fn build(self, mut warning_handler: H) + pub fn build(self, mut warning_handler: H) -> (Result<::layout::LayoutData, FormattingError>, H) { let button_names = self.views.values() @@ -464,7 +464,7 @@ impl Layout { } } -fn create_action( +fn create_action( button_info: &HashMap, name: &str, view_names: Vec<&String>, @@ -494,15 +494,18 @@ fn create_action( (None, None, Some(text)) => SubmitData::Text(text.clone()), (None, None, None) => SubmitData::Text(name.into()), _ => { - warning_handler.handle(&format!( - "Button {} has more than one of (action, keysym, text)", - name - )); + warning_handler.handle( + logging::Level::Warning, + &format!( + "Button {} has more than one of (action, keysym, text)", + name, + ), + ); SubmitData::Text("".into()) }, }; - fn filter_view_name( + fn filter_view_name( button_name: &str, view_name: String, view_names: &Vec<&String>, @@ -511,10 +514,13 @@ fn create_action( if view_names.contains(&&view_name) { view_name } else { - warning_handler.handle(&format!("Button {} switches to missing view {}", - button_name, - view_name, - )); + warning_handler.handle( + logging::Level::Warning, + &format!("Button {} switches to missing view {}", + button_name, + view_name, + ), + ); "base".into() } } @@ -553,27 +559,24 @@ fn create_action( match keysym_valid(keysym.as_str()) { true => keysym.clone(), false => { - warning_handler.handle(&format!( - "Keysym name invalid: {}", - keysym, - )); + warning_handler.handle( + logging::Level::Warning, + &format!( + "Keysym name invalid: {}", + keysym, + ), + ); "space".into() // placeholder }, } )), }, SubmitData::Text(text) => ::action::Action::Submit { - text: { - CString::new(text.clone()) - .map_err(|e| { - warning_handler.handle(&format!( - "Text {} contains problems: {:?}", - text, - e - )); - e - }).ok() - }, + text: CString::new(text.clone()).or_warn( + warning_handler, + logging::Problem::Warning, + &format!("Text {} contains problems", text), + ), keys: text.chars().map(|codepoint| { let codepoint_string = codepoint.to_string(); ::action::KeySym(match keysym_valid(codepoint_string.as_str()) { @@ -587,7 +590,7 @@ fn create_action( /// TODO: Since this will receive user-provided data, /// all .expect() on them should be turned into soft fails -fn create_button( +fn create_button( button_info: &HashMap, outlines: &HashMap, name: &str, @@ -611,14 +614,11 @@ fn create_button( } else if let Some(text) = &button_meta.text { ::layout::Label::Text( CString::new(text.as_str()) - .unwrap_or_else(|e| { - warning_handler.handle(&format!( - "Text {} is invalid: {}", - text, - e, - )); - CString::new("").unwrap() - }) + .or_warn( + warning_handler, + logging::Problem::Warning, + &format!("Text {} is invalid", text), + ).unwrap_or_else(|| CString::new("").unwrap()) ) } else { ::layout::Label::Text(cname.clone()) @@ -629,7 +629,10 @@ fn create_button( if outlines.contains_key(outline) { outline.clone() } else { - warning_handler.handle(&format!("Outline named {} does not exist! Using default for button {}", outline, name)); + warning_handler.handle( + logging::Level::Warning, + &format!("Outline named {} does not exist! Using default for button {}", outline, name) + ); "default".into() } } @@ -638,12 +641,11 @@ fn create_button( let outline = outlines.get(&outline_name) .map(|outline| (*outline).clone()) - .unwrap_or_else(|| { - warning_handler.handle( - &format!("No default outline defined! Using 1x1!") - ); - Outline { width: 1f64, height: 1f64 } - }); + .or_warn( + warning_handler, + logging::Problem::Warning, + "No default outline defined! Using 1x1!", + ).unwrap_or(Outline { width: 1f64, height: 1f64 }); layout::Button { name: cname, @@ -663,7 +665,7 @@ mod tests { use super::*; use std::error::Error as ErrorTrait; - use ::logging::PanicWarn; + use ::logging::ProblemPanic; #[test] fn test_parse_path() { @@ -733,7 +735,7 @@ mod tests { fn test_layout_punctuation() { let out = Layout::from_file(PathBuf::from("tests/layout_key1.yaml")) .unwrap() - .build(PanicWarn).0 + .build(ProblemPanic).0 .unwrap(); assert_eq!( out.views["base"] @@ -748,7 +750,7 @@ mod tests { fn test_layout_unicode() { let out = Layout::from_file(PathBuf::from("tests/layout_key2.yaml")) .unwrap() - .build(PanicWarn).0 + .build(ProblemPanic).0 .unwrap(); assert_eq!( out.views["base"] @@ -764,7 +766,7 @@ mod tests { fn test_layout_unicode_multi() { let out = Layout::from_file(PathBuf::from("tests/layout_key3.yaml")) .unwrap() - .build(PanicWarn).0 + .build(ProblemPanic).0 .unwrap(); assert_eq!( out.views["base"] @@ -779,7 +781,7 @@ mod tests { #[test] fn parsing_fallback() { assert!(Layout::from_resource(FALLBACK_LAYOUT_NAME) - .map(|layout| layout.build(PanicWarn).0.unwrap()) + .map(|layout| layout.build(ProblemPanic).0.unwrap()) .is_ok() ); } @@ -827,7 +829,7 @@ mod tests { }, ".", Vec::new(), - &mut PanicWarn, + &mut ProblemPanic, ), ::action::Action::Submit { text: Some(CString::new(".").unwrap()), @@ -840,7 +842,7 @@ mod tests { fn test_layout_margins() { let out = Layout::from_file(PathBuf::from("tests/layout_margins.yaml")) .unwrap() - .build(PanicWarn).0 + .build(ProblemPanic).0 .unwrap(); assert_eq!( out.margins, diff --git a/src/logging.rs b/src/logging.rs index 7beb9ff1..ba0ed2b8 100644 --- a/src/logging.rs +++ b/src/logging.rs @@ -26,13 +26,15 @@ * 4. logging to an immutable destination type * * Same as above, except it can be parallelized. + * Logs being outputs, they get returned + * instead of being misleadingly passed back through arguments. * It seems more difficult to pass the logger around, * but this may be a solved problem from the area of functional programming. * * This library generally aims at the approach in 3. * */ -use std::error::Error; +use std::fmt::Display; /// Levels are not in order. pub enum Level { @@ -66,18 +68,72 @@ pub enum Level { Debug, } -/// Sugar for logging errors in results. -/// Approach 2. -pub trait Warn { - type Value; - fn or_warn(self, msg: &str) -> Option; +impl Level { + fn as_str(&self) -> &'static str { + match self { + Level::Panic => "Panic", + Level::Bug => "Bug", + Level::Error => "Error", + Level::Warning => "Warning", + Level::Surprise => "Surprise", + Level::Info => "Info", + Level::Debug => "Debug", + } + } } -impl Warn for Result { +impl From for Level { + fn from(problem: Problem) -> Level { + use self::Level::*; + match problem { + Problem::Panic => Panic, + Problem::Bug => Bug, + Problem::Error => Error, + Problem::Warning => Warning, + Problem::Surprise => Surprise, + } + } +} + +/// Only levels which indicate problems +/// To use with `Result::Err` handlers, +/// which are needed only when something went off the optimal path. +/// A separate type ensures that `Err` +/// can't end up misclassified as a benign event like `Info`. +pub enum Problem { + Panic, + Bug, + Error, + Warning, + Surprise, +} + +/// Sugar for logging errors in results. +pub trait Warn where Self: Sized{ + type Value; + /// Approach 2. + fn or_print(self, level: Problem, message: &str) -> Option { + self.or_warn(&mut Print {}, level.into(), message) + } + /// Approach 3. + fn or_warn( + self, + handler: &mut H, + level: Problem, + message: &str, + ) -> Option; +} + +impl Warn for Result { type Value = T; - fn or_warn(self, msg: &str) -> Option { + fn or_warn( + self, + handler: &mut H, + level: Problem, + message: &str, + ) -> Option { self.map_err(|e| { - eprintln!("{}: {}", msg, e); + handler.handle(level.into(), &format!("{}: {}", message, e)); e }).ok() } @@ -85,9 +141,14 @@ impl Warn for Result { impl Warn for Option { type Value = T; - fn or_warn(self, msg: &str) -> Option { + fn or_warn( + self, + handler: &mut H, + level: Problem, + message: &str, + ) -> Option { self.or_else(|| { - eprintln!("{}", msg); + handler.handle(level.into(), message); None }) } @@ -95,26 +156,34 @@ impl Warn for Option { /// A mutable handler for text warnings. /// Approach 3. -pub trait WarningHandler { - /// Handle a warning - fn handle(&mut self, warning: &str); +pub trait Handler { + /// Handle a log message + fn handle(&mut self, level: Level, message: &str); } -/// Prints warnings to stderr -pub struct PrintWarnings; +/// Prints info to stdout, everything else to stderr +pub struct Print; -impl WarningHandler for PrintWarnings { - fn handle(&mut self, warning: &str) { - eprintln!("{}", warning); +impl Handler for Print { + fn handle(&mut self, level: Level, message: &str) { + match level { + Level::Info => println!("Info: {}", message), + l => eprintln!("{}: {}", l.as_str(), message), + } } } -/// Warning handler that will panic at any warning. +/// Warning handler that will panic +/// at any warning, error, surprise, bug, or panic. /// Don't use except in tests -pub struct PanicWarn; +pub struct ProblemPanic; -impl WarningHandler for PanicWarn { - fn handle(&mut self, warning: &str) { - panic!("{}", warning); +impl Handler for ProblemPanic { + fn handle(&mut self, level: Level, message: &str) { + use self::Level::*; + match level { + Panic | Bug | Error | Warning | Surprise => panic!("{}", message), + l => Print{}.handle(l, message), + } } } diff --git a/src/popover.rs b/src/popover.rs index c0b53086..6132b4ab 100644 --- a/src/popover.rs +++ b/src/popover.rs @@ -7,6 +7,7 @@ use ::layout::c::{ Bounds, EekGtkKeyboard }; use ::locale; use ::locale::{ OwnedTranslation, Translation, compare_current_locale }; use ::locale_config::system_locale; +use ::logging; use ::manager; use ::resources; @@ -242,10 +243,13 @@ fn translate_layout_names(layouts: &Vec) -> Vec { .as_ref() .to_owned() ) - .or_warn("No locale detected") + .or_print(logging::Problem::Surprise, "No locale detected") .and_then(|lang| { resources::get_layout_names(lang.as_str()) - .or_warn(&format!("No translations for locale {}", lang)) + .or_print( + logging::Problem::Surprise, + &format!("No translations for locale {}", lang), + ) }); match builtin_translations { diff --git a/src/style.rs b/src/style.rs index 381dd045..2e033a7e 100644 --- a/src/style.rs +++ b/src/style.rs @@ -19,6 +19,7 @@ /*! CSS data loading. */ use std::env; +use ::logging; use glib::object::ObjectExt; use logging::Warn; @@ -94,15 +95,13 @@ fn get_theme_name(settings: >k::Settings) -> GtkTheme { None => GtkTheme { name: { settings.get_property("gtk-theme-name") - // maybe TODO: is this worth a warning? - .or_warn("No theme name") + .or_print(logging::Problem::Surprise, "No theme name") .and_then(|value| value.get::()) .unwrap_or(DEFAULT_THEME_NAME.into()) }, variant: { settings.get_property("gtk-application-prefer-dark-theme") - // maybe TODO: is this worth a warning? - .or_warn("No settings key") + .or_print(logging::Problem::Surprise, "No settings key") .and_then(|value| value.get::()) .and_then(|dark_preferred| match dark_preferred { true => Some("dark".into()), diff --git a/src/tests.rs b/src/tests.rs index 3de54ccf..568fe29a 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -1,17 +1,22 @@ /*! Testing functionality */ use ::data::Layout; +use ::logging; use xkbcommon::xkb; -use ::logging::WarningHandler; - pub struct CountAndPrint(u32); -impl WarningHandler for CountAndPrint { - fn handle(&mut self, warning: &str) { - self.0 = self.0 + 1; - println!("{}", warning); +impl logging::Handler for CountAndPrint { + fn handle(&mut self, level: logging::Level, warning: &str) { + use logging::Level::*; + match level { + Panic | Bug | Error | Warning | Surprise => { + self.0 += 1; + }, + _ => {} + } + logging::Print{}.handle(level, warning) } } @@ -34,7 +39,7 @@ fn check_layout(layout: Layout) { let (layout, handler) = layout.build(handler); if handler.0 > 0 { - println!("{} mistakes in layout", handler.0) + println!("{} problems while parsing layout", handler.0) } let layout = layout.expect("layout broken");