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
Use logging instead of print statements #1862
Conversation
Overall this PR looks good to me. I see a WIP on your title, are there other files that you want to update? |
Codecov Report
@@ Coverage Diff @@
## master #1862 +/- ##
=========================================
Coverage ? 91.12%
=========================================
Files ? 243
Lines ? 31252
Branches ? 3273
=========================================
Hits ? 28478
Misses ? 2041
Partials ? 733
|
@@ -39,9 +39,9 @@ class FetcherError(Exception): | |||
|
|||
def _log(msg): |
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.
Do we really want to keep this function? Since you are using logging
module, you should delete it.
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.
It's useful as shorthand. I just pushed a clarification of the docstring.
Now also rebased. |
Hello @arokem, Thank you for updating !
Comment last updated at 2019-12-19 17:34:36 UTC |
I'm also taking the opportunity to remove |
I think we should keep |
Hi @arokem, What is your status on this PR? it looks close to being done! |
Just rebased. IIUC, the main thing that needs to be done is reinstating |
Yes, I think so |
OK. I believe that I have restored the |
I believe that this ready for a re-review. Codecov is just being dumb. |
@arokem will look into nicer printing options for logging |
Some more thoughts about logging: https://fangpenlin.com/posts/2012/08/26/good-logging-practice-in-python/ |
Is there anything you want to add? I forgot what we said during the last online meeting concerning this PR but it looks ready to be merged for me. Maybe, the logging "design/string format" can be done on a new PR. What do you think? |
Ready on my end. @Garyfallidis : have you had a chance to think about this more? |
can you rebase this one or resolve the conflict @arokem? |
Rebased! |
Thank you @arokem! Any other comments? will merge it by the end of the week |
Hello @arokem. Sorry can you rebase this one more time? Will look into merging the following days. |
Remove all of the `verbose` flags, since this should be handled at the logging level.
All set.
…On Thu, Dec 19, 2019 at 9:19 AM Eleftherios Garyfallidis < ***@***.***> wrote:
Hello @arokem <https://github.com/arokem>. Sorry can you rebase this one
more time? Will look into merging the following days.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1862>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA46NVELKR5OX2LQSMWZILQZOULTANCNFSM4HZJHFCQ>
.
|
Also removing "from future" imports along the way.