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

Dirreke · 2024-02-11T16:55:40Z

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

codecov-commenter · 2024-02-11T16:58:11Z

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

Codecov Report

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

Project coverage is 63.68%. Comparing base (f688e38) to head (d0d35b8).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ppend/rolling_file/policy/compound/trigger/time.rs	88.57%	4 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.42%   63.68%   +0.26%     
==========================================
  Files          25       25              
  Lines        1572     1589      +17     
==========================================
+ Hits          997     1012      +15     
- Misses        575      577       +2

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

bconn98

Couple questions, but otherwise looks good

src/append/rolling_file/policy/compound/trigger/time.rs

bconn98 · 2024-02-16T19:09:13Z

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 · 2024-03-09T04:34:58Z

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

Dirreke · 2024-03-09T15:53:28Z

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

bconn98 · 2024-03-09T16:25:01Z

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

Dirreke · 2024-03-09T17:01:35Z

I will fix it tomorrow.

bconn98

Please make sure to bump the minimum required chrono.

src/append/rolling_file/policy/compound/trigger/time.rs

bconn98

Great work! 🎉

Hellager · 2024-03-27T15:30:33Z

Is there any progress about this PR?

bconn98 · 2024-03-27T19:00:21Z

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

Hellager · 2024-03-29T16:58:31Z

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

src/append/rolling_file/policy/compound/trigger/time.rs

estk · 2024-04-01T16:45:11Z

src/append/rolling_file/policy/compound/trigger/time.rs

-            next_time + Duration::seconds(random_delay as i64)
-        } else {
-            next_time
-        };


Why was this deleted?

Can I close this consersation?

The answer to this is tied into the "wondering" question. Lets wait until estk is back next week. Let's target issues first for future efforts to ensure implications and impacts are considered up front.

estk · 2024-04-01T16:48:49Z

src/append/rolling_file/policy/compound/trigger/time.rs

+            let result = time
+                + Duration::try_hours(increment)
+                    .ok_or(TimeTrigerError::TooLargeInterval(interval))?;
+            return Ok(result);


Please refactor this and other instances into a helper function:

let dur = try_from_hours(increment)?; return Ok(time + dur);

How about using macros?

src/append/rolling_file/policy/compound/trigger/time.rs

estk · 2024-04-01T16:54:15Z

Wondering the reason for this:

Transfer time trigger initialization to the first trigger() execution

Dirreke · 2024-04-01T17:06:03Z

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 · 2024-04-18T12:35:10Z

Awesome, any update? 🥳

bconn98 · 2024-04-18T16:05:31Z

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

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.
Move when the trigger is initialized.
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 · 2024-04-18T16:08:00Z

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 · 2024-04-19T06:32:13Z

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

Dirreke · 2024-05-03T06:23:47Z

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.

Dirreke requested review from estk and gadunga as code owners February 11, 2024 16:55

bconn98 reviewed Feb 16, 2024

View reviewed changes

src/append/rolling_file/policy/compound/trigger/time.rs Outdated Show resolved Hide resolved

src/append/rolling_file/policy/compound/trigger/time.rs Show resolved Hide resolved

bconn98 previously approved these changes Feb 16, 2024

View reviewed changes

Dirreke dismissed bconn98’s stale review via 89f27a7 March 4, 2024 02:54

Dirreke force-pushed the refactor-timetrigger branch from dc3e3fa to 89f27a7 Compare March 4, 2024 02:54

bconn98 previously approved these changes Mar 8, 2024

View reviewed changes

bconn98 enabled auto-merge (squash) March 8, 2024 02:55

auto-merge was automatically disabled March 9, 2024 16:22
Head branch was pushed to by a user without write access

Dirreke dismissed bconn98’s stale review via eb9a7bd March 9, 2024 16:22

Dirreke force-pushed the refactor-timetrigger branch from eb9a7bd to 1db6e18 Compare March 9, 2024 16:25

Dirreke force-pushed the refactor-timetrigger branch from 1db6e18 to 0b8f045 Compare March 10, 2024 04:09

bconn98 reviewed Mar 11, 2024

View reviewed changes

src/append/rolling_file/policy/compound/trigger/time.rs Outdated Show resolved Hide resolved

Dirreke force-pushed the refactor-timetrigger branch from 1db83e0 to df5e6a5 Compare March 11, 2024 06:08

bconn98 previously approved these changes Mar 12, 2024

View reviewed changes

estk reviewed Apr 1, 2024

View reviewed changes

src/append/rolling_file/policy/compound/trigger/time.rs Show resolved Hide resolved

estk reviewed Apr 1, 2024

View reviewed changes

src/append/rolling_file/policy/compound/trigger/time.rs Outdated Show resolved Hide resolved

estk reviewed Apr 1, 2024

View reviewed changes

src/append/rolling_file/policy/compound/trigger/time.rs Outdated Show resolved Hide resolved

Dirreke added 2 commits April 2, 2024 01:26

Update time.rs

7c64ee0

bump chrono

0e56e18

Dirreke force-pushed the refactor-timetrigger branch from df5e6a5 to 0e56e18 Compare April 1, 2024 17:26

Dirreke dismissed bconn98’s stale review via cf6ae27 April 2, 2024 08:47

Dirreke force-pushed the refactor-timetrigger branch 2 times, most recently from 29d8dbd to e475695 Compare April 2, 2024 09:12

update time.rs

d0d35b8

Dirreke force-pushed the refactor-timetrigger branch from e475695 to d0d35b8 Compare April 2, 2024 10:57

bconn98 mentioned this pull request Apr 4, 2024

Resolve Deprecated Chrono API #366

Open

bconn98 mentioned this pull request Apr 18, 2024

TimeTriggerConfig pub #370

Open

bconn98 mentioned this pull request Jul 10, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Dirreke commented Feb 11, 2024

codecov-commenter commented Feb 11, 2024 •

edited

Loading

bconn98 left a comment

bconn98 commented Feb 16, 2024 via email

bconn98 commented Mar 9, 2024

Dirreke commented Mar 9, 2024 •

edited

Loading

bconn98 commented Mar 9, 2024

Dirreke commented Mar 9, 2024

bconn98 left a comment

bconn98 left a comment

Hellager commented Mar 27, 2024

bconn98 commented Mar 27, 2024

Hellager commented Mar 29, 2024

estk Apr 1, 2024

Dirreke Apr 2, 2024

bconn98 Apr 3, 2024

estk Apr 1, 2024

Dirreke Apr 2, 2024

estk commented Apr 1, 2024

Dirreke commented Apr 1, 2024

carlocorradini commented Apr 18, 2024

bconn98 commented Apr 18, 2024

Dirreke commented Apr 18, 2024 •

edited

Loading

carlocorradini commented Apr 19, 2024

Dirreke commented May 3, 2024 •

edited

Loading

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

Are you sure you want to change the base?

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

Conversation

Dirreke commented Feb 11, 2024

codecov-commenter commented Feb 11, 2024 • edited Loading

Codecov Report

bconn98 left a comment

Choose a reason for hiding this comment

bconn98 commented Feb 16, 2024 via email

bconn98 commented Mar 9, 2024

Dirreke commented Mar 9, 2024 • edited Loading

bconn98 commented Mar 9, 2024

Dirreke commented Mar 9, 2024

bconn98 left a comment

Choose a reason for hiding this comment

bconn98 left a comment

Choose a reason for hiding this comment

Hellager commented Mar 27, 2024

bconn98 commented Mar 27, 2024

Hellager commented Mar 29, 2024

estk Apr 1, 2024

Choose a reason for hiding this comment

Dirreke Apr 2, 2024

Choose a reason for hiding this comment

bconn98 Apr 3, 2024

Choose a reason for hiding this comment

estk Apr 1, 2024

Choose a reason for hiding this comment

Dirreke Apr 2, 2024

Choose a reason for hiding this comment

estk commented Apr 1, 2024

Dirreke commented Apr 1, 2024

carlocorradini commented Apr 18, 2024

bconn98 commented Apr 18, 2024

Dirreke commented Apr 18, 2024 • edited Loading

carlocorradini commented Apr 19, 2024

Dirreke commented May 3, 2024 • edited Loading

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

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

codecov-commenter commented Feb 11, 2024 •

edited

Loading

Dirreke commented Mar 9, 2024 •

edited

Loading

Dirreke commented Apr 18, 2024 •

edited

Loading

Dirreke commented May 3, 2024 •

edited

Loading