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

filter::parse_spec() ignore bogon empty, blank (sub)strings #188

Closed salewski closed 3 years ago

salewski commented 3 years ago

This PR fixes an issue with filter::parse_spec() accepting as legit some empty and blank spec (sub)strings that it should be ignoring.

There are two commits:

  1. The first adds five unit tests that demonstrate the issue (three of them fail).

  2. The second commit contains the simple fix, which basically extends the intent of the existing code (ignore invalid spec strings) to cover these additional scenarios. With this change, all of the tests pass.

salewski commented 3 years ago

Just a heads-up that the "Check formatting" CI test is flagging an issue unrelated to this changeset[0].

I would fix that up in a separate PR, but since it wants to remove a trailing comma I'm not even sure if that change is wanted. Leaving the trailing commas seems to be more idiomatic, so maybe the lint needs tuning instead?

FWIW, this is what it is saying:

    Run actions-rs/cargo@v1
    /usr/share/rust/.cargo/bin/cargo fmt -- --check
    Diff in /home/runner/work/env_logger/env_logger/src/lib.rs at line 231:

     #![doc(
         html_logo_url = "https://www.rust-lang.org/logos/rust-logo-128x128-blk-v2.png",
    -    html_favicon_url = "https://www.rust-lang.org/static/images/favicon.ico",
    +    html_favicon_url = "https://www.rust-lang.org/static/images/favicon.ico"
     )]
     #![cfg_attr(test, deny(warnings))]
     // When compiled for the rustc compiler itself we want to make sure that this is
    Error: The process '/usr/share/rust/.cargo/bin/cargo' failed with exit code 1

[0]: It did flag some whitespace changes in my original push, but I fixed those up and rebased. Now the above item is the only thing it is complaining about.

salewski commented 3 years ago

I just rebased this and force pushed, just to pull in the rustfmt fix and re-trigger the CI checks. I'm expecting them to all go now.