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

Create an example of logging with file rotation #295

Merged
merged 7 commits into from Feb 2, 2024
Merged

Conversation

c-git
Copy link
Contributor

@c-git c-git commented Dec 14, 2022

I've also added a small snippet at the bottom to generate log message. I can remove this if it is not desirable

@c-git
Copy link
Contributor Author

c-git commented Oct 29, 2023

Was there something more I needed to do on this? Sorry I don't remember.

@bconn98
Copy link
Collaborator

bconn98 commented Dec 25, 2023

@c-git Can you please rebase and add your example to the Cargo.toml

@c-git
Copy link
Contributor Author

c-git commented Dec 25, 2023

I didn't mean to request reviews I just clicked update. Haven't quite figured out how to also do the edit from my phone. If I don't figure it out I'll do it after the holidays when I'm back on my computer.

@bconn98
Copy link
Collaborator

bconn98 commented Dec 25, 2023

No worries, just happened to take a look. If you could also fix the magic numbers that'd be great

@c-git
Copy link
Contributor Author

c-git commented Dec 27, 2023

I moved the configurable parts of the demo to the top of the file (I hope those are the numbers you were referring to).

I don't know which features are required to be included in the Cargo.toml. I tried none and it worked when I ran cargo run --example log_to_file_with_rolling so I don't know if I'm testing it wrong. But I checked the docs and guessed which would be needed.

@bconn98
Copy link
Collaborator

bconn98 commented Dec 27, 2023

Your testing was correct, because all of the features you're using are part of the default list you technically do not need to list any features. However, it's following convention to list them out so it looks good.

bconn98
bconn98 previously approved these changes Dec 27, 2023
@c-git
Copy link
Contributor Author

c-git commented Jan 18, 2024

Let me know if there is anything else I need to do?

@bconn98
Copy link
Collaborator

bconn98 commented Jan 19, 2024

No actions from me. I'll see what I can do

@bconn98
Copy link
Collaborator

bconn98 commented Jan 26, 2024

@c-git Please resolve the conflicts, then we are good to go.

@c-git
Copy link
Contributor Author

c-git commented Jan 26, 2024

I think I fixed it but did it from the GitHub UI so not quite sure but it doesn't show the conflict anymore

Cargo.toml Outdated Show resolved Hide resolved
Co-authored-by: Bryan Conn <30739012+bconn98@users.noreply.github.com>
@c-git
Copy link
Contributor Author

c-git commented Jan 27, 2024

Sorry about that, I'll try to see if I can open up more context next time. Seems like an obvious mistake in hindsight.

@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b0ad8ba) 61.77% compared to head (f953789) 61.77%.

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #295   +/-   ##
=======================================
  Coverage   61.77%   61.77%           
=======================================
  Files          23       23           
  Lines        1431     1431           
=======================================
  Hits          884      884           
  Misses        547      547           

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

@bconn98
Copy link
Collaborator

bconn98 commented Jan 28, 2024

@c-git Just need you to fix that CI job

@c-git
Copy link
Contributor Author

c-git commented Jan 28, 2024

Ok I'll try to check it. Will probably be tomorrow night.

@c-git
Copy link
Contributor Author

c-git commented Jan 28, 2024

Ok I think I fixed it I ran cargo +nightly fmt and committed those changes

@bconn98 bconn98 enabled auto-merge (squash) January 28, 2024 18:39
@bconn98 bconn98 disabled auto-merge February 2, 2024 02:34
@bconn98
Copy link
Collaborator

bconn98 commented Feb 2, 2024

@c-git, hey the MSRV needs to get bumped to support the toml crate. Here is a commit making the same change.

@c-git
Copy link
Contributor Author

c-git commented Feb 2, 2024

@c-git, hey the MSRV needs to get bumped to support the toml crate. Here is a commit making the same change.

Hey @bconn98 I'm a bit lost. I don't follow the connection between the example in this PR and the toml crate. What do you need me to do?

@bconn98
Copy link
Collaborator

bconn98 commented Feb 2, 2024 via email

@estk estk merged commit a898a07 into estk:main Feb 2, 2024
12 checks passed
@c-git c-git deleted the patch-1 branch February 2, 2024 06:05
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.

None yet

4 participants