-
Notifications
You must be signed in to change notification settings - Fork 0
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
Replace print with loguru #10
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.
Quick and easy implementation. Some comments in-line.
topofileformats/asd.py
Outdated
@@ -21,6 +23,9 @@ | |||
skip_bytes, | |||
) | |||
|
|||
logger.remove() | |||
logger.add(sys.stderr, format="<white>{time}</white> | <level>{level}</level> | <blue>{message}</blue>") |
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.
I also pedantically think the main error message should be white, use colour on other sections but the log message
are the thing we want to be able to read easiest and they stand out best in consoles when they are white.
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.
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.
Quick and easy implementation. Some comments in-line.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
==========================================
+ Coverage 65.87% 66.76% +0.88%
==========================================
Files 3 5 +2
Lines 337 346 +9
==========================================
+ Hits 222 231 +9
Misses 115 115 ☔ View full report in Codecov by Sentry. |
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.
Thanks for finding the time to continue working on this @SylviaWhittle .
I've one suggestion which is to setup the logger in __init__.py
rather than its own file and noticed a very minor tidying to a couple of lines.
Co-authored-by: Neil Shephard <[email protected]>
Did some [reading](https://loguru.readthedocs.io/en/stable/resources/recipes.html#configuring-loguru-to-be-used-by-a-library-or-an-application) and I was wrong to suggest setting up `loguru` in `__init__.py` so happy to use `topofileforamts/logging.py`. Implemented the one recommendation in the above linked section. No tests for logging but logs _are_ output.
174035b
to
e1b1392
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.
This is good to go @SylviaWhittle 👍
Closes #2
This PR replaces all the
print
statements with logging fromloguru
(colour coded too)