Skip to content

Commit c42511c

Browse files
MaximkaaaMaximkaaa
committed
refactor: Return errors from parse_spec 
1 parent 0e25d9e commit c42511c

File tree

2 files changed

+139
-47
lines changed

2 files changed

+139
-47
lines changed

‎crates/env_filter/src/filter.rs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ use log::{LevelFilter, Metadata, Record};
66

77
use crate::enabled;
88
use crate::parse_spec;
9+
use crate::parser::ParseResult;
910
use crate::Directive;
1011
use crate::FilterOp;
1112

@@ -97,7 +98,17 @@ impl Builder {
9798
///
9899
/// [Enabling Logging]: ../index.html#enabling-logging
99100
pub fn parse(&mut self, filters: &str) -> &mut Self {
100-
let (directives, filter) = parse_spec(filters);
101+
#![allow(clippy::print_stderr)] // compatibility
102+
103+
let ParseResult {
104+
directives,
105+
filter,
106+
errors,
107+
} = parse_spec(filters);
108+
109+
for error in errors {
110+
eprintln!("warning: {error}, ignoring it");
111+
}
101112

102113
self.filter = filter;
103114

‎crates/env_filter/src/parser.rs

Lines changed: 127 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -3,23 +3,38 @@ use log::LevelFilter;
33
use crate::Directive;
44
use crate::FilterOp;
55

6+
#[derive(Default, Debug)]
7+
pub(crate) struct ParseResult {
8+
pub(crate) directives: Vec<Directive>,
9+
pub(crate) filter: Option<FilterOp>,
10+
pub(crate) errors: Vec<String>,
11+
}
12+
13+
impl ParseResult {
14+
fn add_directive(&mut self, directive: Directive) {
15+
self.directives.push(directive);
16+
}
17+
18+
fn set_filter(&mut self, filter: FilterOp) {
19+
self.filter = Some(filter);
20+
}
21+
22+
fn add_error(&mut self, message: String) {
23+
self.errors.push(message);
24+
}
25+
}
26+
627
/// Parse a logging specification string (e.g: `crate1,crate2::mod3,crate3::x=error/foo`)
728
/// and return a vector with log directives.
8-
pub(crate) fn parse_spec(spec: &str) -> (Vec<Directive>, Option<FilterOp>) {
9-
#![allow(clippy::print_stderr)] // compatibility
10-
11-
let mut dirs = Vec::new();
29+
pub(crate) fn parse_spec(spec: &str) -> ParseResult {
30+
let mut result = ParseResult::default();
1231

1332
let mut parts = spec.split('/');
1433
let mods = parts.next();
1534
let filter = parts.next();
1635
if parts.next().is_some() {
17-
eprintln!(
18-
"warning: invalid logging spec '{}', \
19-
ignoring it (too many '/'s)",
20-
spec
21-
);
22-
return (dirs, None);
36+
result.add_error(format!("invalid logging spec '{}' (too many '/'s)", spec));
37+
return result;
2338
}
2439
if let Some(m) = mods {
2540
for s in m.split(',').map(|ss| ss.trim()) {
@@ -42,50 +57,47 @@ pub(crate) fn parse_spec(spec: &str) -> (Vec<Directive>, Option<FilterOp>) {
4257
if let Ok(num) = part1.parse() {
4358
(num, Some(part0))
4459
} else {
45-
eprintln!(
46-
"warning: invalid logging spec '{}', \
47-
ignoring it",
48-
part1
49-
);
60+
result.add_error(format!("invalid logging spec '{}'", part1));
5061
continue;
5162
}
5263
}
5364
_ => {
54-
eprintln!(
55-
"warning: invalid logging spec '{}', \
56-
ignoring it",
57-
s
58-
);
65+
result.add_error(format!("invalid logging spec '{}'", s));
5966
continue;
6067
}
6168
};
62-
dirs.push(Directive {
69+
70+
result.add_directive(Directive {
6371
name: name.map(|s| s.to_owned()),
6472
level: log_level,
6573
});
6674
}
6775
}
6876

69-
let filter = filter.and_then(|filter| match FilterOp::new(filter) {
70-
Ok(re) => Some(re),
71-
Err(e) => {
72-
eprintln!("warning: invalid regex filter - {}", e);
73-
None
77+
if let Some(filter) = filter {
78+
match FilterOp::new(filter) {
79+
Ok(filter_op) => result.set_filter(filter_op),
80+
Err(err) => result.add_error(format!("invalid regex filter - {}", err)),
7481
}
75-
});
82+
}
7683

77-
(dirs, filter)
84+
result
7885
}
7986

8087
#[cfg(test)]
8188
mod tests {
8289
use log::LevelFilter;
8390

84-
use super::parse_spec;
91+
use super::{parse_spec, ParseResult};
8592

8693
#[test]
8794
fn parse_spec_valid() {
88-
let (dirs, filter) = parse_spec("crate1::mod1=error,crate1::mod2,crate2=debug");
95+
let ParseResult {
96+
directives: dirs,
97+
filter,
98+
..
99+
} = parse_spec("crate1::mod1=error,crate1::mod2,crate2=debug");
100+
89101
assert_eq!(dirs.len(), 3);
90102
assert_eq!(dirs[0].name, Some("crate1::mod1".to_owned()));
91103
assert_eq!(dirs[0].level, LevelFilter::Error);
@@ -101,7 +113,12 @@ mod tests {
101113
#[test]
102114
fn parse_spec_invalid_crate() {
103115
// test parse_spec with multiple = in specification
104-
let (dirs, filter) = parse_spec("crate1::mod1=warn=info,crate2=debug");
116+
let ParseResult {
117+
directives: dirs,
118+
filter,
119+
..
120+
} = parse_spec("crate1::mod1=warn=info,crate2=debug");
121+
105122
assert_eq!(dirs.len(), 1);
106123
assert_eq!(dirs[0].name, Some("crate2".to_owned()));
107124
assert_eq!(dirs[0].level, LevelFilter::Debug);
@@ -111,7 +128,12 @@ mod tests {
111128
#[test]
112129
fn parse_spec_invalid_level() {
113130
// test parse_spec with 'noNumber' as log level
114-
let (dirs, filter) = parse_spec("crate1::mod1=noNumber,crate2=debug");
131+
let ParseResult {
132+
directives: dirs,
133+
filter,
134+
..
135+
} = parse_spec("crate1::mod1=noNumber,crate2=debug");
136+
115137
assert_eq!(dirs.len(), 1);
116138
assert_eq!(dirs[0].name, Some("crate2".to_owned()));
117139
assert_eq!(dirs[0].level, LevelFilter::Debug);
@@ -121,7 +143,12 @@ mod tests {
121143
#[test]
122144
fn parse_spec_string_level() {
123145
// test parse_spec with 'warn' as log level
124-
let (dirs, filter) = parse_spec("crate1::mod1=wrong,crate2=warn");
146+
let ParseResult {
147+
directives: dirs,
148+
filter,
149+
..
150+
} = parse_spec("crate1::mod1=wrong,crate2=warn");
151+
125152
assert_eq!(dirs.len(), 1);
126153
assert_eq!(dirs[0].name, Some("crate2".to_owned()));
127154
assert_eq!(dirs[0].level, LevelFilter::Warn);
@@ -131,7 +158,12 @@ mod tests {
131158
#[test]
132159
fn parse_spec_empty_level() {
133160
// test parse_spec with '' as log level
134-
let (dirs, filter) = parse_spec("crate1::mod1=wrong,crate2=");
161+
let ParseResult {
162+
directives: dirs,
163+
filter,
164+
..
165+
} = parse_spec("crate1::mod1=wrong,crate2=");
166+
135167
assert_eq!(dirs.len(), 1);
136168
assert_eq!(dirs[0].name, Some("crate2".to_owned()));
137169
assert_eq!(dirs[0].level, LevelFilter::max());
@@ -141,7 +173,11 @@ mod tests {
141173
#[test]
142174
fn parse_spec_empty_level_isolated() {
143175
// test parse_spec with "" as log level (and the entire spec str)
144-
let (dirs, filter) = parse_spec(""); // should be ignored
176+
let ParseResult {
177+
directives: dirs,
178+
filter,
179+
..
180+
} = parse_spec(""); // should be ignored
145181
assert_eq!(dirs.len(), 0);
146182
assert!(filter.is_none());
147183
}
@@ -150,7 +186,11 @@ mod tests {
150186
fn parse_spec_blank_level_isolated() {
151187
// test parse_spec with a white-space-only string specified as the log
152188
// level (and the entire spec str)
153-
let (dirs, filter) = parse_spec(" "); // should be ignored
189+
let ParseResult {
190+
directives: dirs,
191+
filter,
192+
..
193+
} = parse_spec(" "); // should be ignored
154194
assert_eq!(dirs.len(), 0);
155195
assert!(filter.is_none());
156196
}
@@ -160,7 +200,11 @@ mod tests {
160200
// The spec should contain zero or more comma-separated string slices,
161201
// so a comma-only string should be interpreted as two empty strings
162202
// (which should both be treated as invalid, so ignored).
163-
let (dirs, filter) = parse_spec(","); // should be ignored
203+
let ParseResult {
204+
directives: dirs,
205+
filter,
206+
..
207+
} = parse_spec(","); // should be ignored
164208
assert_eq!(dirs.len(), 0);
165209
assert!(filter.is_none());
166210
}
@@ -171,7 +215,11 @@ mod tests {
171215
// so this bogus spec should be interpreted as containing one empty
172216
// string and one blank string. Both should both be treated as
173217
// invalid, so ignored.
174-
let (dirs, filter) = parse_spec(", "); // should be ignored
218+
let ParseResult {
219+
directives: dirs,
220+
filter,
221+
..
222+
} = parse_spec(", "); // should be ignored
175223
assert_eq!(dirs.len(), 0);
176224
assert!(filter.is_none());
177225
}
@@ -182,15 +230,23 @@ mod tests {
182230
// so this bogus spec should be interpreted as containing one blank
183231
// string and one empty string. Both should both be treated as
184232
// invalid, so ignored.
185-
let (dirs, filter) = parse_spec(" ,"); // should be ignored
233+
let ParseResult {
234+
directives: dirs,
235+
filter,
236+
..
237+
} = parse_spec(" ,"); // should be ignored
186238
assert_eq!(dirs.len(), 0);
187239
assert!(filter.is_none());
188240
}
189241

190242
#[test]
191243
fn parse_spec_global() {
192244
// test parse_spec with no crate
193-
let (dirs, filter) = parse_spec("warn,crate2=debug");
245+
let ParseResult {
246+
directives: dirs,
247+
filter,
248+
..
249+
} = parse_spec("warn,crate2=debug");
194250
assert_eq!(dirs.len(), 2);
195251
assert_eq!(dirs[0].name, None);
196252
assert_eq!(dirs[0].level, LevelFilter::Warn);
@@ -202,7 +258,11 @@ mod tests {
202258
#[test]
203259
fn parse_spec_global_bare_warn_lc() {
204260
// test parse_spec with no crate, in isolation, all lowercase
205-
let (dirs, filter) = parse_spec("warn");
261+
let ParseResult {
262+
directives: dirs,
263+
filter,
264+
..
265+
} = parse_spec("warn");
206266
assert_eq!(dirs.len(), 1);
207267
assert_eq!(dirs[0].name, None);
208268
assert_eq!(dirs[0].level, LevelFilter::Warn);
@@ -212,7 +272,11 @@ mod tests {
212272
#[test]
213273
fn parse_spec_global_bare_warn_uc() {
214274
// test parse_spec with no crate, in isolation, all uppercase
215-
let (dirs, filter) = parse_spec("WARN");
275+
let ParseResult {
276+
directives: dirs,
277+
filter,
278+
..
279+
} = parse_spec("WARN");
216280
assert_eq!(dirs.len(), 1);
217281
assert_eq!(dirs[0].name, None);
218282
assert_eq!(dirs[0].level, LevelFilter::Warn);
@@ -222,7 +286,11 @@ mod tests {
222286
#[test]
223287
fn parse_spec_global_bare_warn_mixed() {
224288
// test parse_spec with no crate, in isolation, mixed case
225-
let (dirs, filter) = parse_spec("wArN");
289+
let ParseResult {
290+
directives: dirs,
291+
filter,
292+
..
293+
} = parse_spec("wArN");
226294
assert_eq!(dirs.len(), 1);
227295
assert_eq!(dirs[0].name, None);
228296
assert_eq!(dirs[0].level, LevelFilter::Warn);
@@ -231,7 +299,11 @@ mod tests {
231299

232300
#[test]
233301
fn parse_spec_valid_filter() {
234-
let (dirs, filter) = parse_spec("crate1::mod1=error,crate1::mod2,crate2=debug/abc");
302+
let ParseResult {
303+
directives: dirs,
304+
filter,
305+
..
306+
} = parse_spec("crate1::mod1=error,crate1::mod2,crate2=debug/abc");
235307
assert_eq!(dirs.len(), 3);
236308
assert_eq!(dirs[0].name, Some("crate1::mod1".to_owned()));
237309
assert_eq!(dirs[0].level, LevelFilter::Error);
@@ -246,7 +318,12 @@ mod tests {
246318

247319
#[test]
248320
fn parse_spec_invalid_crate_filter() {
249-
let (dirs, filter) = parse_spec("crate1::mod1=error=warn,crate2=debug/a.c");
321+
let ParseResult {
322+
directives: dirs,
323+
filter,
324+
..
325+
} = parse_spec("crate1::mod1=error=warn,crate2=debug/a.c");
326+
250327
assert_eq!(dirs.len(), 1);
251328
assert_eq!(dirs[0].name, Some("crate2".to_owned()));
252329
assert_eq!(dirs[0].level, LevelFilter::Debug);
@@ -255,7 +332,11 @@ mod tests {
255332

256333
#[test]
257334
fn parse_spec_empty_with_filter() {
258-
let (dirs, filter) = parse_spec("crate1/a*c");
335+
let ParseResult {
336+
directives: dirs,
337+
filter,
338+
..
339+
} = parse_spec("crate1/a*c");
259340
assert_eq!(dirs.len(), 1);
260341
assert_eq!(dirs[0].name, Some("crate1".to_owned()));
261342
assert_eq!(dirs[0].level, LevelFilter::max());

0 commit comments

Comments
 (0)