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

[Dirreke]

• Delete struct TimeTriggerConfig for lib without config_parsing feature.
• Transfer time trigger initialization to the first trigger() execution

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 9.09091% with 20 lines in your changes missing coverage. Please review.

Project coverage is 61.60%. Comparing base (8ab1b34) to head (04d5d08).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
...ppend/rolling_file/policy/compound/trigger/time.rs	9.09%	20 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #347      +/-   ##
==========================================
- Coverage   63.39%   61.60%   -1.80%     
==========================================
  Files          24       25       +1     
  Lines        1557     1625      +68     
==========================================
+ Hits          987     1001      +14     
- Misses        570      624      +54     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gauntl3t12]

Couple questions, but otherwise looks good

[gauntl3t12]

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.

[gauntl3t12]

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

[Dirreke]

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

[gauntl3t12]

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

[Dirreke]

I will fix it tomorrow.

[gauntl3t12]

Please make sure to bump the minimum required chrono.

[gauntl3t12]

Great work! 🎉

[Hellager]

Is there any progress about this PR?

[gauntl3t12]

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

[Hellager]

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

[estk]

Wondering the reason for this:

Transfer time trigger initialization to the first trigger() execution

[Dirreke]

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]

Awesome, any update? 🥳

[gauntl3t12]

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

1. Fix the deprecated API fix: replace deprecated API in chrono 0.4.35 for converting to time #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]

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]

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

[Dirreke]

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.

[estk]

@Dirreke can you please rebase this on main

[Dirreke]

Of course. I'll do it later today.