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

Adds options for the TimedRotatingFileHandler #40

Merged
merged 4 commits into from
Nov 9, 2023
Merged

Adds options for the TimedRotatingFileHandler #40

merged 4 commits into from
Nov 9, 2023

Conversation

havok2063
Copy link
Contributor

This PR adds options to start_file_logger for the TimedRotatingFileHandler. It makes when an explicit option, and adds utc and atTime as options as well, as described here.

@havok2063 havok2063 added the enhancement New feature or request label Nov 9, 2023
@havok2063 havok2063 self-assigned this Nov 9, 2023
@havok2063 havok2063 requested a review from albireox as a code owner November 9, 2023 19:59
@albireox
Copy link
Member

albireox commented Nov 9, 2023

This looks good. Can you get the tests pass? You can just lint the file with black and that should work. And maybe add a test or two?

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (67a19c7) 93.74% compared to head (d47b759) 93.27%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
- Coverage   93.74%   93.27%   -0.46%     
==========================================
  Files           5        5              
  Lines         479      505      +26     
  Branches      109      114       +5     
==========================================
+ Hits          449      471      +22     
- Misses         14       16       +2     
- Partials       16       18       +2     
Files Coverage Δ
src/sdsstools/logger.py 86.18% <33.34%> (-0.92%) ⬇️

... and 1 file with indirect coverage changes

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

@havok2063
Copy link
Contributor Author

Done. I added a simple test on the options for now. I wanted to add a test on the rotation without adding an explicit time.sleep call to the test, but couldn't get the mocker and logger to register my mocked times correctly. Then I just time-boxed it.

Copy link
Member

@albireox albireox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I'll merge and release a new version; I have a few changes I also want to get published.

@albireox albireox merged commit df7283a into main Nov 9, 2023
5 of 6 checks passed
@albireox albireox deleted the timedfh branch November 9, 2023 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants