search

estk / log4rs

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

Refactor `time.rs` to make the code logic the same as others. #347

Open Dirreke opened 5 months ago

Dirreke commented 5 months ago
codecov-commenter commented 5 months ago

Codecov Report

Attention: Patch coverage is 91.66667% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 63.91%. Comparing base (8ab1b34) to head (df5e6a5).

Files Patch % Lines
...ppend/rolling_file/policy/compound/trigger/time.rs 91.66% 4 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 #347 +/- ## ========================================== + Coverage 63.39% 63.91% +0.52% ========================================== Files 24 24 Lines 1557 1588 +31 ========================================== + Hits 987 1015 +28 - Misses 570 573 +3 ```

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

bconn98 commented 5 months ago

Sorry, I hadn’t used that feature before and misread the docs when I pulled them up the first time. I’m in agreement, looks good.

bconn98 commented 4 months ago

Looks like chronic changed their time interface. Can you fix that well you're already in there for this PR? @Dirreke

Dirreke commented 4 months ago

Which choice do you prefer if the duration time as millis exceed i64::MAX? return panic? or just use i64::MAX.

bconn98 commented 4 months ago

Let’s return an error, similar to how the size trigger does it

Dirreke commented 4 months ago

I will fix it tomorrow.

Hellager commented 3 months ago

Is there any progress about this PR?

bconn98 commented 3 months ago

I’m communicating with the repo owner to get this merged in. It’s g2g from me.

Hellager commented 3 months ago

Got it, hopes it will be soon and thanks for your great work.

estk commented 3 months ago

Wondering the reason for this:

Transfer time trigger initialization to the first trigger() execution
Dirreke commented 3 months ago

If we build it in program instead of config file, we have to build TimeTrigger first before initiating builder. The time recorded should be the time when the first log message is written but not the time when TimeTrigger be built. Another reason is just that I want to keep it the same as the onstartup trigger.

carlocorradini commented 3 months ago

Awesome, any update? 🥳

bconn98 commented 3 months ago

@carlocorradini The owner of the repo requested that we break up this PR into separate pieces.

  1. Fix the deprecated API #367. This is waiting on getting the owners time again to review.
  2. Move when the trigger is initialized.
  3. Refactor the code to include making the config field public.

This will allow the changes to be analyzed independently and makes the impact of each change more easily tracked. @Dirreke Have you been able to work on breaking number 2 out into a separate PR?

Dirreke commented 3 months ago

Thanks for that. However, I'm a little busy these days. Maybe I will do it next week. Or maybe I can rebase it after #367 merged

carlocorradini commented 3 months ago

Firstly, awesome 😎 And... What do you think if we use the builder pattern to populate the fields in config?

Dirreke commented 2 months ago

2. Move when the trigger is initialized.

Task 2 has a lot of code related to task 1. I'd like to finish it after #367 is merged.

For Issue #370, I open #372 to provide a quick PR to solve it.