rust-cli / env_logger

A logging implementation for `log` which is configured via an environment variable.
https://docs.rs/env_logger
Apache License 2.0
803 stars 125 forks source link

Builder#parse_env does not override manually set filter level #196

Closed BramBonne closed 3 years ago

BramBonne commented 3 years ago

Consider the following example (runnable version):

std::env::set_var("RUST_LOG", "debug");

let mut builder = env_logger::Builder::new();
builder.filter_level(log::LevelFilter::Trace);

// The following call should override the log level to Debug, as read from the environment
builder.parse_default_env();

builder.init();
assert_eq!(log::max_level(), log::LevelFilter::Trace);

According to the documentation, the parse_env function "allows a builder to be configured with default parameters, to be then overridden by the environment". For this example, that would mean that log::max_level() should be set to Debug at the end of this example. In reality, it's set to Trace instead.

Removing the call to builder.filter_level(log::LevelFilter::Trace) correctly sets the log level to Debug instead.

jplatte commented 3 years ago

Yeah, the docs are pretty clear here. A PR fixing this would be welcome.

BramBonne commented 3 years ago

I provided both #199 and #200 as alternative potential fixes to this problem, depending on whether we want directive names to be unique in general (#200), or not (#199).

BramBonne commented 3 years ago

Hey @jplatte, friendly ping on this. Would #200 be an acceptable way to fix this, or is there a better approach? Thanks for taking a look :-)

Kind regards, Bram

mainrs commented 3 years ago

I'll have time on the weekend to take a look at the PR.

BramBonne commented 3 years ago

Thank you! But no rush, I was just checking it was still being considered 🙂

mainrs commented 3 years ago

@jplatte I've taken a look at both of the PRs. Both look good.

I don't have strong preference for either PR, but if I'd have to choose I'd say we should merge #200, as it means more cosistent behaviour across the library in general by always overwriting previous directives. I can't think of an example off the top of my head where this might not be wanted.

jplatte commented 3 years ago

I've had a look, I agree that #200 seems like the better solution.

BramBonne commented 3 years ago

Hey,

Just checking if anything is expected/needed from my end here.

Kind regards, Bram

jplatte commented 3 years ago

No. I merged #200. Sorry about the long delay.