-
Notifications
You must be signed in to change notification settings - Fork 456
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
refactor(logger): Make logger less verbose #1295
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1295 +/- ##
==========================================
+ Coverage 78.96% 80.33% +1.37%
==========================================
Files 262 264 +2
Lines 4801 4805 +4
Branches 578 577 -1
==========================================
+ Hits 3791 3860 +69
+ Misses 856 797 -59
+ Partials 154 148 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@BreadGenie Added some more suggestions to #1293 |
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 looking pretty good, but I've got a few requests.
cve_bin_tool/cvedb.py
Outdated
@@ -252,7 +252,9 @@ async def refresh(self): | |||
os.makedirs(self.cachedir) | |||
# check for the latest version | |||
if self.version_check: | |||
self.LOGGER.info("Checking if there is a newer version.") | |||
self.LOGGER.info( | |||
f"cve-bin-tool is running {VERSION}. Checking if there is a newer version." |
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.
Let's put
self.LOGGER.info(f"CVE Binary Tool version {VERSION}")
above line 254 so it prints the version even if the version check is disabled. We might want to move it up so that it's the first thing printed when cli is run. I think it's useful information to have in the console output.
I'm not sure we really need to report that were doing the check at all, only if it discovers that there's a newer version. Do we need to async check_latest_version so that the tool doesn't pause here? (the check is barely perceptable on my machine, but I have a fairly fast internet connection so it's possible that it's more noticeable to others).
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 works pretty fast for me too (my network speed is sometimes in kbps). I think we might not need 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.
Failing on (unrelated) NVD tests, can be merged.
Fix #1293
I have covered the contents in the issue and I'm now looking for other improvements.