From c42511cc3cb31d4d6e908af7ac33a7cd9ce8674a Mon Sep 17 00:00:00 2001 From: Maxim Date: Sat, 20 Jul 2024 23:34:15 +0900 Subject: [PATCH] refactor: Return errors from `parse_spec` --- crates/env_filter/src/filter.rs | 13 ++- crates/env_filter/src/parser.rs | 173 +++++++++++++++++++++++--------- 2 files changed, 139 insertions(+), 47 deletions(-) diff --git a/crates/env_filter/src/filter.rs b/crates/env_filter/src/filter.rs index 5ae018d..f0f860f 100644 --- a/crates/env_filter/src/filter.rs +++ b/crates/env_filter/src/filter.rs @@ -6,6 +6,7 @@ use log::{LevelFilter, Metadata, Record}; use crate::enabled; use crate::parse_spec; +use crate::parser::ParseResult; use crate::Directive; use crate::FilterOp; @@ -97,7 +98,17 @@ impl Builder { /// /// [Enabling Logging]: ../index.html#enabling-logging pub fn parse(&mut self, filters: &str) -> &mut Self { - let (directives, filter) = parse_spec(filters); + #![allow(clippy::print_stderr)] // compatibility + + let ParseResult { + directives, + filter, + errors, + } = parse_spec(filters); + + for error in errors { + eprintln!("warning: {error}, ignoring it"); + } self.filter = filter; diff --git a/crates/env_filter/src/parser.rs b/crates/env_filter/src/parser.rs index 3cb01be..846b3ce 100644 --- a/crates/env_filter/src/parser.rs +++ b/crates/env_filter/src/parser.rs @@ -3,23 +3,38 @@ use log::LevelFilter; use crate::Directive; use crate::FilterOp; +#[derive(Default, Debug)] +pub(crate) struct ParseResult { + pub(crate) directives: Vec, + pub(crate) filter: Option, + pub(crate) errors: Vec, +} + +impl ParseResult { + fn add_directive(&mut self, directive: Directive) { + self.directives.push(directive); + } + + fn set_filter(&mut self, filter: FilterOp) { + self.filter = Some(filter); + } + + fn add_error(&mut self, message: String) { + self.errors.push(message); + } +} + /// Parse a logging specification string (e.g: `crate1,crate2::mod3,crate3::x=error/foo`) /// and return a vector with log directives. -pub(crate) fn parse_spec(spec: &str) -> (Vec, Option) { - #![allow(clippy::print_stderr)] // compatibility - - let mut dirs = Vec::new(); +pub(crate) fn parse_spec(spec: &str) -> ParseResult { + let mut result = ParseResult::default(); let mut parts = spec.split('/'); let mods = parts.next(); let filter = parts.next(); if parts.next().is_some() { - eprintln!( - "warning: invalid logging spec '{}', \ - ignoring it (too many '/'s)", - spec - ); - return (dirs, None); + result.add_error(format!("invalid logging spec '{}' (too many '/'s)", spec)); + return result; } if let Some(m) = mods { for s in m.split(',').map(|ss| ss.trim()) { @@ -42,50 +57,47 @@ pub(crate) fn parse_spec(spec: &str) -> (Vec, Option) { if let Ok(num) = part1.parse() { (num, Some(part0)) } else { - eprintln!( - "warning: invalid logging spec '{}', \ - ignoring it", - part1 - ); + result.add_error(format!("invalid logging spec '{}'", part1)); continue; } } _ => { - eprintln!( - "warning: invalid logging spec '{}', \ - ignoring it", - s - ); + result.add_error(format!("invalid logging spec '{}'", s)); continue; } }; - dirs.push(Directive { + + result.add_directive(Directive { name: name.map(|s| s.to_owned()), level: log_level, }); } } - let filter = filter.and_then(|filter| match FilterOp::new(filter) { - Ok(re) => Some(re), - Err(e) => { - eprintln!("warning: invalid regex filter - {}", e); - None + if let Some(filter) = filter { + match FilterOp::new(filter) { + Ok(filter_op) => result.set_filter(filter_op), + Err(err) => result.add_error(format!("invalid regex filter - {}", err)), } - }); + } - (dirs, filter) + result } #[cfg(test)] mod tests { use log::LevelFilter; - use super::parse_spec; + use super::{parse_spec, ParseResult}; #[test] fn parse_spec_valid() { - let (dirs, filter) = parse_spec("crate1::mod1=error,crate1::mod2,crate2=debug"); + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec("crate1::mod1=error,crate1::mod2,crate2=debug"); + assert_eq!(dirs.len(), 3); assert_eq!(dirs[0].name, Some("crate1::mod1".to_owned())); assert_eq!(dirs[0].level, LevelFilter::Error); @@ -101,7 +113,12 @@ mod tests { #[test] fn parse_spec_invalid_crate() { // test parse_spec with multiple = in specification - let (dirs, filter) = parse_spec("crate1::mod1=warn=info,crate2=debug"); + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec("crate1::mod1=warn=info,crate2=debug"); + assert_eq!(dirs.len(), 1); assert_eq!(dirs[0].name, Some("crate2".to_owned())); assert_eq!(dirs[0].level, LevelFilter::Debug); @@ -111,7 +128,12 @@ mod tests { #[test] fn parse_spec_invalid_level() { // test parse_spec with 'noNumber' as log level - let (dirs, filter) = parse_spec("crate1::mod1=noNumber,crate2=debug"); + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec("crate1::mod1=noNumber,crate2=debug"); + assert_eq!(dirs.len(), 1); assert_eq!(dirs[0].name, Some("crate2".to_owned())); assert_eq!(dirs[0].level, LevelFilter::Debug); @@ -121,7 +143,12 @@ mod tests { #[test] fn parse_spec_string_level() { // test parse_spec with 'warn' as log level - let (dirs, filter) = parse_spec("crate1::mod1=wrong,crate2=warn"); + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec("crate1::mod1=wrong,crate2=warn"); + assert_eq!(dirs.len(), 1); assert_eq!(dirs[0].name, Some("crate2".to_owned())); assert_eq!(dirs[0].level, LevelFilter::Warn); @@ -131,7 +158,12 @@ mod tests { #[test] fn parse_spec_empty_level() { // test parse_spec with '' as log level - let (dirs, filter) = parse_spec("crate1::mod1=wrong,crate2="); + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec("crate1::mod1=wrong,crate2="); + assert_eq!(dirs.len(), 1); assert_eq!(dirs[0].name, Some("crate2".to_owned())); assert_eq!(dirs[0].level, LevelFilter::max()); @@ -141,7 +173,11 @@ mod tests { #[test] fn parse_spec_empty_level_isolated() { // test parse_spec with "" as log level (and the entire spec str) - let (dirs, filter) = parse_spec(""); // should be ignored + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec(""); // should be ignored assert_eq!(dirs.len(), 0); assert!(filter.is_none()); } @@ -150,7 +186,11 @@ mod tests { fn parse_spec_blank_level_isolated() { // test parse_spec with a white-space-only string specified as the log // level (and the entire spec str) - let (dirs, filter) = parse_spec(" "); // should be ignored + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec(" "); // should be ignored assert_eq!(dirs.len(), 0); assert!(filter.is_none()); } @@ -160,7 +200,11 @@ mod tests { // The spec should contain zero or more comma-separated string slices, // so a comma-only string should be interpreted as two empty strings // (which should both be treated as invalid, so ignored). - let (dirs, filter) = parse_spec(","); // should be ignored + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec(","); // should be ignored assert_eq!(dirs.len(), 0); assert!(filter.is_none()); } @@ -171,7 +215,11 @@ mod tests { // so this bogus spec should be interpreted as containing one empty // string and one blank string. Both should both be treated as // invalid, so ignored. - let (dirs, filter) = parse_spec(", "); // should be ignored + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec(", "); // should be ignored assert_eq!(dirs.len(), 0); assert!(filter.is_none()); } @@ -182,7 +230,11 @@ mod tests { // so this bogus spec should be interpreted as containing one blank // string and one empty string. Both should both be treated as // invalid, so ignored. - let (dirs, filter) = parse_spec(" ,"); // should be ignored + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec(" ,"); // should be ignored assert_eq!(dirs.len(), 0); assert!(filter.is_none()); } @@ -190,7 +242,11 @@ mod tests { #[test] fn parse_spec_global() { // test parse_spec with no crate - let (dirs, filter) = parse_spec("warn,crate2=debug"); + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec("warn,crate2=debug"); assert_eq!(dirs.len(), 2); assert_eq!(dirs[0].name, None); assert_eq!(dirs[0].level, LevelFilter::Warn); @@ -202,7 +258,11 @@ mod tests { #[test] fn parse_spec_global_bare_warn_lc() { // test parse_spec with no crate, in isolation, all lowercase - let (dirs, filter) = parse_spec("warn"); + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec("warn"); assert_eq!(dirs.len(), 1); assert_eq!(dirs[0].name, None); assert_eq!(dirs[0].level, LevelFilter::Warn); @@ -212,7 +272,11 @@ mod tests { #[test] fn parse_spec_global_bare_warn_uc() { // test parse_spec with no crate, in isolation, all uppercase - let (dirs, filter) = parse_spec("WARN"); + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec("WARN"); assert_eq!(dirs.len(), 1); assert_eq!(dirs[0].name, None); assert_eq!(dirs[0].level, LevelFilter::Warn); @@ -222,7 +286,11 @@ mod tests { #[test] fn parse_spec_global_bare_warn_mixed() { // test parse_spec with no crate, in isolation, mixed case - let (dirs, filter) = parse_spec("wArN"); + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec("wArN"); assert_eq!(dirs.len(), 1); assert_eq!(dirs[0].name, None); assert_eq!(dirs[0].level, LevelFilter::Warn); @@ -231,7 +299,11 @@ mod tests { #[test] fn parse_spec_valid_filter() { - let (dirs, filter) = parse_spec("crate1::mod1=error,crate1::mod2,crate2=debug/abc"); + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec("crate1::mod1=error,crate1::mod2,crate2=debug/abc"); assert_eq!(dirs.len(), 3); assert_eq!(dirs[0].name, Some("crate1::mod1".to_owned())); assert_eq!(dirs[0].level, LevelFilter::Error); @@ -246,7 +318,12 @@ mod tests { #[test] fn parse_spec_invalid_crate_filter() { - let (dirs, filter) = parse_spec("crate1::mod1=error=warn,crate2=debug/a.c"); + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec("crate1::mod1=error=warn,crate2=debug/a.c"); + assert_eq!(dirs.len(), 1); assert_eq!(dirs[0].name, Some("crate2".to_owned())); assert_eq!(dirs[0].level, LevelFilter::Debug); @@ -255,7 +332,11 @@ mod tests { #[test] fn parse_spec_empty_with_filter() { - let (dirs, filter) = parse_spec("crate1/a*c"); + let ParseResult { + directives: dirs, + filter, + .. + } = parse_spec("crate1/a*c"); assert_eq!(dirs.len(), 1); assert_eq!(dirs[0].name, Some("crate1".to_owned())); assert_eq!(dirs[0].level, LevelFilter::max());