estk / log4rs

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

fix: replace deprecated API in chrono 0.4.35 for converting to time #367

Open bconn98 opened 3 months ago

bconn98 commented 3 months ago

Replace deprecated time conversion API from chrono crate.

Deprecation occured in v0.4.35. Work performed by @Dirreke Separated out by @bconn98 Resolves #366

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 88.88889% with 3 lines in your changes missing coverage. Please review.

Project coverage is 63.88%. Comparing base (8ab1b34) to head (9acd373). Report is 2 commits behind head on main.

Files Patch % Lines
...ppend/rolling_file/policy/compound/trigger/time.rs 88.88% 3 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 #367 +/- ## ========================================== + Coverage 63.39% 63.88% +0.49% ========================================== Files 24 25 +1 Lines 1557 1581 +24 ========================================== + Hits 987 1010 +23 - Misses 570 571 +1 ```

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

Dirreke commented 3 months ago

and we can add this test

    #[test]
    fn trigger_large_interval() {
        let interval = TimeTriggerInterval::Second(i64::MAX);
        let file = tempfile::tempdir().unwrap();
        let logfile = LogFile {
            writer: &mut None,
            path: file.path(),
            len: 0,
        };
        let config = TimeTriggerConfig {
            interval,
            ..Default::default()
        };

        let trigger = TimeTrigger::new(config);
        let error = trigger.trigger(&logfile).unwrap_err();
        let box_dyn = Box::<dyn StdError>::from(error);
        assert_eq!(
            format!("The integer value {:?} for the specified time trigger interval is too large, it must be less than 9,223,372,036,854,775,807 seconds.", interval),
            box_dyn.to_string()
        );
    }
bconn98 commented 3 months ago

and we can add this test

@Dirreke Doesn't look like it's doable without some of your refactor. Based on needing to unwrap the error in the new method vs unwrapping in the trigger. If you see another option let me know. I messed around with catch_unwind but without a concrete error type that wasn't working for me.

bconn98 commented 2 weeks ago

@estk it looks like Codecov wiped out your access token. Can you try and fix that?

Dirreke commented 1 week ago

Thanks for the code! ♥️♥️

estk commented 1 week ago

Just wanted to add some more context here. It makes sense to use failable versions of for example Duration::weeks(), the only thing is it needs to be accompanied by an error handling strategy that gracefully presents that error and provides the context for the user to fix that error.

Previously by using the panicking version of these methods the error handling is easy, it just unwinds and gives the user (optionally) a stack trace to go find what happened. Aspirationally we would be able to use the failable methods and direct the user to the data that caused the issue instead of the code that caught the issue. (This is what would be possible in your current approach.)

However, if we're going to go thru the trouble of preventing a panic, we need to ensure the usability and debuggability is at least as good as it was when we panicked. As to how we do this... idk, but I'm certain we should alert the user that their input was incorrect instead of simply selecting some arbitrary default. IMO, without holistically rethinking our approach to presenting errors to users it will be difficult to present errors well without panicking.

bconn98 commented 1 week ago

FWIW the get_next_time error would be one that we generate, so I think we can/do/will when I fix the IntervalError provide some data to the user.

Bigger picture, we can setup a milestone + branch to start working towards a 2.0 version that includes the API change for multiple rollers and an informative but not panicking solution.

bconn98 commented 1 week ago

Closes #367