From c42511cc3cb31d4d6e908af7ac33a7cd9ce8674a Mon Sep 17 00:00:00 2001 From: Maxim Date: Sat, 20 Jul 2024 23:34:15 +0900 Subject: [PATCH 1/3] 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()); From faf5b3e4998b2a5babf44e3dac7f6ebe1a85c4c0 Mon Sep 17 00:00:00 2001 From: Maxim Date: Sat, 20 Jul 2024 23:47:42 +0900 Subject: [PATCH 2/3] chore: Tests for `parse_spec` error messages --- Cargo.lock | 35 ++++++++ crates/env_filter/Cargo.toml | 3 + crates/env_filter/src/parser.rs | 152 ++++++++++++++++++++++++++++---- 3 files changed, 173 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 05bdc0c..4579123 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -71,6 +71,7 @@ version = "0.1.0" dependencies = [ "log", "regex", + "snapbox", ] [[package]] @@ -102,6 +103,12 @@ version = "2.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2dffe52ecf27772e601905b7522cb4ef790d2cc203488bbd0e2fe85fcb74566d" +[[package]] +name = "normalize-line-endings" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "61807f77802ff30975e01f4f071c8ba10c022052f98b3294119f3e615d13e5be" + [[package]] name = "regex" version = "1.7.0" @@ -119,6 +126,34 @@ version = "0.6.28" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "456c603be3e8d448b072f410900c09faf164fbce2d480456f50eea6e25f9c848" +[[package]] +name = "similar" +version = "2.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fa42c91313f1d05da9b26f267f931cf178d4aba455b4c4622dd7355eb80c6640" + +[[package]] +name = "snapbox" +version = "0.6.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0d656960fa127e80ade23c321d8c573bb9ba462c3a69e62ede635fc180ffc6cc" +dependencies = [ + "anstream", + "anstyle", + "normalize-line-endings", + "similar", + "snapbox-macros", +] + +[[package]] +name = "snapbox-macros" +version = "0.3.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b1f4c14672714436c09254801c934b203196a51182a5107fb76591c7cc56424d" +dependencies = [ + "anstream", +] + [[package]] name = "utf8parse" version = "0.2.1" diff --git a/crates/env_filter/Cargo.toml b/crates/env_filter/Cargo.toml index 94e864c..ba984da 100644 --- a/crates/env_filter/Cargo.toml +++ b/crates/env_filter/Cargo.toml @@ -33,5 +33,8 @@ regex = ["dep:regex"] log = { version = "0.4.8", features = ["std"] } regex = { version = "1.0.3", optional = true, default-features=false, features=["std", "perf"] } +[dev-dependencies] +snapbox = "0.6" + [lints] workspace = true diff --git a/crates/env_filter/src/parser.rs b/crates/env_filter/src/parser.rs index 846b3ce..b957e4a 100644 --- a/crates/env_filter/src/parser.rs +++ b/crates/env_filter/src/parser.rs @@ -87,6 +87,7 @@ pub(crate) fn parse_spec(spec: &str) -> ParseResult { #[cfg(test)] mod tests { use log::LevelFilter; + use snapbox::{assert_data_eq, str}; use super::{parse_spec, ParseResult}; @@ -95,7 +96,7 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = parse_spec("crate1::mod1=error,crate1::mod2,crate2=debug"); assert_eq!(dirs.len(), 3); @@ -108,6 +109,8 @@ mod tests { assert_eq!(dirs[2].name, Some("crate2".to_owned())); assert_eq!(dirs[2].level, LevelFilter::Debug); assert!(filter.is_none()); + + assert!(errors.is_empty()); } #[test] @@ -116,13 +119,19 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = 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); assert!(filter.is_none()); + + assert_eq!(errors.len(), 1); + assert_data_eq!( + &errors[0], + str!["invalid logging spec 'crate1::mod1=warn=info'"] + ); } #[test] @@ -131,13 +140,16 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = 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); assert!(filter.is_none()); + + assert_eq!(errors.len(), 1); + assert_data_eq!(&errors[0], str!["invalid logging spec 'noNumber'"]); } #[test] @@ -146,13 +158,16 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = 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); assert!(filter.is_none()); + + assert_eq!(errors.len(), 1); + assert_data_eq!(&errors[0], str!["invalid logging spec 'wrong'"]); } #[test] @@ -161,13 +176,16 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = 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()); assert!(filter.is_none()); + + assert_eq!(errors.len(), 1); + assert_data_eq!(&errors[0], str!["invalid logging spec 'wrong'"]); } #[test] @@ -176,10 +194,11 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = parse_spec(""); // should be ignored assert_eq!(dirs.len(), 0); assert!(filter.is_none()); + assert!(errors.is_empty()); } #[test] @@ -189,10 +208,11 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = parse_spec(" "); // should be ignored assert_eq!(dirs.len(), 0); assert!(filter.is_none()); + assert!(errors.is_empty()); } #[test] @@ -203,10 +223,11 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = parse_spec(","); // should be ignored assert_eq!(dirs.len(), 0); assert!(filter.is_none()); + assert!(errors.is_empty()); } #[test] @@ -218,10 +239,11 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = parse_spec(", "); // should be ignored assert_eq!(dirs.len(), 0); assert!(filter.is_none()); + assert!(errors.is_empty()); } #[test] @@ -233,10 +255,11 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = parse_spec(" ,"); // should be ignored assert_eq!(dirs.len(), 0); assert!(filter.is_none()); + assert!(errors.is_empty()); } #[test] @@ -245,7 +268,7 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = parse_spec("warn,crate2=debug"); assert_eq!(dirs.len(), 2); assert_eq!(dirs[0].name, None); @@ -253,6 +276,7 @@ mod tests { assert_eq!(dirs[1].name, Some("crate2".to_owned())); assert_eq!(dirs[1].level, LevelFilter::Debug); assert!(filter.is_none()); + assert!(errors.is_empty()); } #[test] @@ -261,12 +285,13 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = parse_spec("warn"); assert_eq!(dirs.len(), 1); assert_eq!(dirs[0].name, None); assert_eq!(dirs[0].level, LevelFilter::Warn); assert!(filter.is_none()); + assert!(errors.is_empty()); } #[test] @@ -275,12 +300,13 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = parse_spec("WARN"); assert_eq!(dirs.len(), 1); assert_eq!(dirs[0].name, None); assert_eq!(dirs[0].level, LevelFilter::Warn); assert!(filter.is_none()); + assert!(errors.is_empty()); } #[test] @@ -289,12 +315,13 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = parse_spec("wArN"); assert_eq!(dirs.len(), 1); assert_eq!(dirs[0].name, None); assert_eq!(dirs[0].level, LevelFilter::Warn); assert!(filter.is_none()); + assert!(errors.is_empty()); } #[test] @@ -302,7 +329,7 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = 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())); @@ -314,6 +341,7 @@ mod tests { assert_eq!(dirs[2].name, Some("crate2".to_owned())); assert_eq!(dirs[2].level, LevelFilter::Debug); assert!(filter.is_some() && filter.unwrap().to_string() == "abc"); + assert!(errors.is_empty()); } #[test] @@ -321,13 +349,19 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = 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); assert!(filter.is_some() && filter.unwrap().to_string() == "a.c"); + + assert_eq!(errors.len(), 1); + assert_data_eq!( + &errors[0], + str!["invalid logging spec 'crate1::mod1=error=warn'"] + ); } #[test] @@ -335,11 +369,95 @@ mod tests { let ParseResult { directives: dirs, filter, - .. + errors, } = 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()); assert!(filter.is_some() && filter.unwrap().to_string() == "a*c"); + assert!(errors.is_empty()); + } + + #[test] + fn parse_spec_with_multiple_filters() { + let ParseResult { + directives: dirs, + filter, + errors, + } = parse_spec("debug/abc/a.c"); + assert!(dirs.is_empty()); + assert!(filter.is_none()); + + assert_eq!(errors.len(), 1); + assert_data_eq!( + &errors[0], + str!["invalid logging spec 'debug/abc/a.c' (too many '/'s)"] + ); + } + + #[test] + fn parse_spec_multiple_invalid_crates() { + // test parse_spec with multiple = in specification + let ParseResult { + directives: dirs, + filter, + errors, + } = parse_spec("crate1::mod1=warn=info,crate2=debug,crate3=error=error"); + + assert_eq!(dirs.len(), 1); + assert_eq!(dirs[0].name, Some("crate2".to_owned())); + assert_eq!(dirs[0].level, LevelFilter::Debug); + assert!(filter.is_none()); + + assert_eq!(errors.len(), 2); + assert_data_eq!( + &errors[0], + str!["invalid logging spec 'crate1::mod1=warn=info'"] + ); + assert_data_eq!( + &errors[1], + str!["invalid logging spec 'crate3=error=error'"] + ); + } + + #[test] + fn parse_spec_multiple_invalid_levels() { + // test parse_spec with 'noNumber' as log level + let ParseResult { + directives: dirs, + filter, + errors, + } = parse_spec("crate1::mod1=noNumber,crate2=debug,crate3=invalid"); + + assert_eq!(dirs.len(), 1); + assert_eq!(dirs[0].name, Some("crate2".to_owned())); + assert_eq!(dirs[0].level, LevelFilter::Debug); + assert!(filter.is_none()); + + assert_eq!(errors.len(), 2); + assert_data_eq!(&errors[0], str!["invalid logging spec 'noNumber'"]); + assert_data_eq!(&errors[1], str!["invalid logging spec 'invalid'"]); + } + + #[test] + fn parse_spec_invalid_crate_and_level() { + // test parse_spec with 'noNumber' as log level + let ParseResult { + directives: dirs, + filter, + errors, + } = parse_spec("crate1::mod1=debug=info,crate2=debug,crate3=invalid"); + + assert_eq!(dirs.len(), 1); + assert_eq!(dirs[0].name, Some("crate2".to_owned())); + assert_eq!(dirs[0].level, LevelFilter::Debug); + assert!(filter.is_none()); + + assert_eq!(errors.len(), 2); + assert_data_eq!( + &errors[0], + str!["invalid logging spec 'crate1::mod1=debug=info'"] + ); + assert_data_eq!(&errors[1], str!["invalid logging spec 'invalid'"]); } } From 05aacb93c5193624c664177ad5f473f7efc5c94d Mon Sep 17 00:00:00 2001 From: Maxim Date: Tue, 23 Jul 2024 14:47:46 +0900 Subject: [PATCH 3/3] feat: Add Builder::try_parse method Current implementation of the Builder::parse() method prints out specification errors to stderr and then just ignores them. This is fine for most console applications, but in some cases better control over what is happening is needed: * Sometimes there is no access to stderr, in that case there is no way to find out what went wrong. * For some applications it's more desirable to fail on startup than to run with (partially) invalid configuration. For such cases this commit introduces a new method try_parse that does the same thing as the parse method, but returns an Err in case an error in the specification is found. --- crates/env_filter/CHANGELOG.md | 4 +++ crates/env_filter/src/filter.rs | 37 ++++++++++++++++++++ crates/env_filter/src/lib.rs | 1 + crates/env_filter/src/parser.rs | 60 ++++++++++++++++++++++++++++++++- 4 files changed, 101 insertions(+), 1 deletion(-) diff --git a/crates/env_filter/CHANGELOG.md b/crates/env_filter/CHANGELOG.md index b9a276c..b8c47e8 100644 --- a/crates/env_filter/CHANGELOG.md +++ b/crates/env_filter/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] - ReleaseDate +### Features + +- Added `env_filter::Builder::try_parse(&self, &str)` method (failable version of `env_filter::Builder::parse()`) + ## [0.1.0] - 2024-01-19 diff --git a/crates/env_filter/src/filter.rs b/crates/env_filter/src/filter.rs index f0f860f..fa28ebd 100644 --- a/crates/env_filter/src/filter.rs +++ b/crates/env_filter/src/filter.rs @@ -9,6 +9,7 @@ use crate::parse_spec; use crate::parser::ParseResult; use crate::Directive; use crate::FilterOp; +use crate::ParseError; /// A builder for a log filter. /// @@ -118,6 +119,22 @@ impl Builder { self } + /// Parses the directive string, returning an error if the given directive string is invalid. + /// + /// See the [Enabling Logging] section for more details. + /// + /// [Enabling Logging]: ../index.html#enabling-logging + pub fn try_parse(&mut self, filters: &str) -> Result<&mut Self, ParseError> { + let (directives, filter) = parse_spec(filters).ok()?; + + self.filter = filter; + + for directive in directives { + self.insert_directive(directive); + } + Ok(self) + } + /// Build a log filter. pub fn build(&mut self) -> Filter { assert!(!self.built, "attempt to re-use consumed builder"); @@ -241,6 +258,7 @@ impl fmt::Debug for Filter { #[cfg(test)] mod tests { use log::{Level, LevelFilter}; + use snapbox::{assert_data_eq, str}; use super::{enabled, Builder, Directive, Filter}; @@ -455,6 +473,25 @@ mod tests { } } + #[test] + fn try_parse_valid_filter() { + let logger = Builder::new() + .try_parse("info,crate1::mod1=warn") + .expect("valid filter returned error") + .build(); + assert!(enabled(&logger.directives, Level::Warn, "crate1::mod1")); + assert!(enabled(&logger.directives, Level::Info, "crate2::mod2")); + } + + #[test] + fn try_parse_invalid_filter() { + let error = Builder::new().try_parse("info,crate1=invalid").unwrap_err(); + assert_data_eq!( + error, + str!["error parsing logger filter: invalid logging spec 'invalid'"] + ); + } + #[test] fn match_full_path() { let logger = make_logger_filter(vec![ diff --git a/crates/env_filter/src/lib.rs b/crates/env_filter/src/lib.rs index 6a72204..e9f42d1 100644 --- a/crates/env_filter/src/lib.rs +++ b/crates/env_filter/src/lib.rs @@ -56,3 +56,4 @@ use parser::parse_spec; pub use filter::Builder; pub use filter::Filter; pub use filtered_log::FilteredLog; +pub use parser::ParseError; diff --git a/crates/env_filter/src/parser.rs b/crates/env_filter/src/parser.rs index b957e4a..59596dd 100644 --- a/crates/env_filter/src/parser.rs +++ b/crates/env_filter/src/parser.rs @@ -1,4 +1,6 @@ use log::LevelFilter; +use std::error::Error; +use std::fmt::{Display, Formatter}; use crate::Directive; use crate::FilterOp; @@ -22,8 +24,35 @@ impl ParseResult { fn add_error(&mut self, message: String) { self.errors.push(message); } + + pub(crate) fn ok(self) -> Result<(Vec, Option), ParseError> { + let Self { + directives, + filter, + errors, + } = self; + if let Some(error) = errors.into_iter().next() { + Err(ParseError { details: error }) + } else { + Ok((directives, filter)) + } + } +} + +/// Error during logger directive parsing process. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct ParseError { + details: String, +} + +impl Display for ParseError { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "error parsing logger filter: {}", self.details) + } } +impl Error for ParseError {} + /// 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) -> ParseResult { @@ -86,11 +115,18 @@ pub(crate) fn parse_spec(spec: &str) -> ParseResult { #[cfg(test)] mod tests { + use crate::ParseError; use log::LevelFilter; - use snapbox::{assert_data_eq, str}; + use snapbox::{assert_data_eq, str, Data, IntoData}; use super::{parse_spec, ParseResult}; + impl IntoData for ParseError { + fn into_data(self) -> Data { + self.to_string().into_data() + } + } + #[test] fn parse_spec_valid() { let ParseResult { @@ -460,4 +496,26 @@ mod tests { ); assert_data_eq!(&errors[1], str!["invalid logging spec 'invalid'"]); } + + #[test] + fn parse_error_message_single_error() { + let error = parse_spec("crate1::mod1=debug=info,crate2=debug") + .ok() + .unwrap_err(); + assert_data_eq!( + error, + str!["error parsing logger filter: invalid logging spec 'crate1::mod1=debug=info'"] + ); + } + + #[test] + fn parse_error_message_multiple_errors() { + let error = parse_spec("crate1::mod1=debug=info,crate2=debug,crate3=invalid") + .ok() + .unwrap_err(); + assert_data_eq!( + error, + str!["error parsing logger filter: invalid logging spec 'crate1::mod1=debug=info'"] + ); + } }