-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: optimize the magic number to Option and extract common code #33
chore: optimize the magic number to Option and extract common code #33
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tisonkun , would you mind taking a look this minor refactor in your free time? thank you very much.
821a148
to
a599e65
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One minor comment inline.
Since we touch a few times the rolling file implementation, I'll appreciate it if you can try to port some tests in https://github.com/tokio-rs/tracing/blob/master/tracing-appender/src/rolling.rs so that we can guarantee the rolling behavior kept during changes. |
Signed-off-by: tison <[email protected]>
Thanks for your contribution! |
Thanks for the quick review and merge! Ahhh, I addressed your comment and I'm trying to add some teets on my Mac. I will create a new issue to add test later due to it's already merged. |
@1996fanrui OK. I may be too hurry to merge a PR :D |
Changes:
rolling.State#next_date
from usize toOption<usize>
rolling.State#next_date
is 0 when rotation is Rotation::Never.next_date
is not effective.date.unix_timestamp() as usize
inside ofnext_date
Rotation#next_date
convert the data to timestamp, we can convert it inside ofRotation#next_date
Rotation#next_date
needs to rename toRotation#next_date_timestamp