fix: Replace print statements with logging and logging modular level (PR2)#3648
fix: Replace print statements with logging and logging modular level (PR2)#3648PredictiveManish wants to merge 4 commits intochaoss:mainfrom
Conversation
Signed-off-by: PredictiveManish <manish.tiwari.09@zohomail.in>
Signed-off-by: PredictiveManish <manish.tiwari.09@zohomail.in>
Signed-off-by: PredictiveManish <manish.tiwari.09@zohomail.in>
Try not to put the word "fixes" right before the issue citation since thats what triggers github to link it to the underlying issue in a way that will close it when this merges. its a tricky balance between citing the changes/connecting the issue and still having the multi-part changes work |
MoralCode
left a comment
There was a problem hiding this comment.
while this PR does replace some prints with proper logger calls, It also introduces some less-than-ideal practices (especially global variables) that i think outweigh the benefit of the logs
augur/tasks/git/dependency_libyear_tasks/libyear_util/pypi_libyear_util.py
Outdated
Show resolved
Hide resolved
| logging.debug(f"[{file_name}] Successfully decoded with encoding: {encoding}") | ||
| logger.debug(f"[{file_name}] Successfully decoded with encoding: {encoding}") |
There was a problem hiding this comment.
more global variable logging that needs fixing
There was a problem hiding this comment.
@MoralCode : @ABrain7710 can set me straight if I am not understanding correctly, but our logging is designed to create logs that are distinct to the different data collection utilities/tasks Augur performs. This is one of those tasks. It would create much more object-creation overhead if a logger instance were instantiated in each method for this task. I am assuming that by global you mean global across methods in this file, which is for one task.
I think there may be a trade-off between the downside of all the methods in a task sharing a logger and the cost of creating and destroying logger objects in each method.
There was a problem hiding this comment.
my feedback here is sort of addressing that problem.
youre right that we dont want to create a logger per method. But i also dont want to be using a global variable that all our methods just... use because then the dependencies/expectations each method requires to run are not clearly laid out in the signature.
What I would like to see here is that, instead of getting a logger variable from global scope, the logger would be passed in as a parameter like we do in many other places. Then we can instantiate it when we set up the task and pass it to the entrypoint, which will pass it down through any other functions that get called.
This will also help us run tasks individually in isolated contexts, like unit testing, more easily.
Added logger parameter to get_release_date function. Signed-off-by: Manish Tiwari <manish.tiwari.09@zohomail.in>
Description
This PR standardizes the logging implementation across
libyear_utilto follow Python best practices and solve #3313:Changes Made:
logger = logging.getLogger(__name__)This PR partially solves #3313
Split of #3478
Notes for Reviewers
Testing
logger.infologging.basicConfig(level=INFO).Signed commits