estk / log4rs

A highly configurable logging framework for Rust
Apache License 2.0
973 stars 143 forks source link

DRAFT: refactor: COLOR_MODE to OnceCell #365

Open bconn98 opened 3 months ago

bconn98 commented 3 months ago

Refactor COLOR_MODE to be easier for testing and for using features in the standard library come v1.71.0.

Addresses: #368

codecov-commenter commented 3 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 64.20%. Comparing base (f688e38) to head (863c336). Report is 1 commits behind head on main.

Files Patch % Lines
src/encode/writer/console.rs 93.33% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #365 +/- ## ========================================== + Coverage 63.42% 64.20% +0.78% ========================================== Files 25 25 Lines 1572 1573 +1 ========================================== + Hits 997 1010 +13 + Misses 575 563 -12 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

estk commented 2 weeks ago

Looks like CI is failing on this one still...

bconn98 commented 1 week ago

I see what you're saying but I don't think we get any test benefit with that change (which may be okay because of the integration tests). That said, I don't think I agree that using Lazy is any more lightweight. Both will be initialized once and return a reference on access if I'm reading the docs correctly.

estk commented 1 week ago

@bconn98 by lightweight i mean not having the following copy pasted at every callsite. There are other options if you prefer, you could wrap the Lazy in a struct and in cfg(test) mode internally it could simply be a Mutex that you can mutate as necessary.

            let color_mode_init = {
                let no_color = env::var("NO_COLOR");
                let clicolor_force = env::var("CLICOLOR_FORCE");
                let clicolor = env::var("CLICOLOR");
                get_color_mode(no_color, clicolor_force, clicolor)
            };
bconn98 commented 1 week ago

I tried a couple different versions of using mutex.

  1. Locking the whole tests
  2. Like you suggested a struct with a mutex locked getter (using deref and force)

But no avail, the Lazy will not execute a second time thereby causing tests to fail. I wouldn't want to put the actual ENV values inside of mutex's as that would invalidate the majority of the test.

bconn98 commented 1 week ago

I found a solution that I think is satisfactory. By Option wrapping all the args, we can dependency inject during testing or otherwise just pass Nones and pull from the env. I would like this MR to merge in the initial migration, than a follow on MR will come to bump our MSRV and use the solution that is now part of the std lib.