Skip to content
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

Merged
merged 2 commits into from
Aug 10, 2024

Conversation

1996fanrui
Copy link
Contributor

Changes:

  • Refactor the type of rolling.State#next_date from usize to Option<usize>
    • rolling.State#next_date is 0 when rotation is Rotation::Never.
    • Using Option compared to magic number can intuitively tell developer that next_date is not effective.
  • Move date.unix_timestamp() as usize inside of next_date
    • All callers of Rotation#next_date convert the data to timestamp, we can convert it inside of Rotation#next_date
    • Also, Rotation#next_date needs to rename to Rotation#next_date_timestamp

Copy link
Contributor Author

@1996fanrui 1996fanrui left a 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.

@1996fanrui 1996fanrui force-pushed the refactor/rolling-file-magic-number branch from 821a148 to a599e65 Compare August 10, 2024 05:48
Copy link
Contributor

@tisonkun tisonkun left a 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.

@tisonkun
Copy link
Contributor

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]>
@tisonkun tisonkun enabled auto-merge (squash) August 10, 2024 06:54
@tisonkun
Copy link
Contributor

Thanks for your contribution!

@tisonkun tisonkun merged commit d2db4c7 into fast:main Aug 10, 2024
3 checks passed
@1996fanrui
Copy link
Contributor Author

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.

@tisonkun
Copy link
Contributor

@1996fanrui OK. I may be too hurry to merge a PR :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants