Skip to content

Commit

Permalink
refactor: Return errors from parse_spec
Browse files Browse the repository at this point in the history
  • Loading branch information
@Maximkaaa
Maximkaaa committed Jul 20, 2024
1 parent 0e25d9e commit c42511c
Show file tree
Hide file tree
Showing 2 changed files with 139 additions and 47 deletions.
13 changes: 12 additions & 1 deletion crates/env_filter/src/filter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down
173 changes: 127 additions & 46 deletions crates/env_filter/src/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,38 @@ use log::LevelFilter;
use crate::Directive;
use crate::FilterOp;

#[derive(Default, Debug)]
pub(crate) struct ParseResult {
pub(crate) directives: Vec<Directive>,
pub(crate) filter: Option<FilterOp>,
pub(crate) errors: Vec<String>,
}

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<Directive>, Option<FilterOp>) {
#![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()) {
Expand All @@ -42,50 +57,47 @@ pub(crate) fn parse_spec(spec: &str) -> (Vec<Directive>, Option<FilterOp>) {
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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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());
Expand All @@ -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());
}
Expand All @@ -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());
}
Expand All @@ -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());
}
Expand All @@ -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());
}
Expand All @@ -182,15 +230,23 @@ 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());
}

#[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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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());
Expand Down

0 comments on commit c42511c

Please sign in to comment.