-
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
cvedb will scrap data now #908
Conversation
Codecov Report
@@ Coverage Diff @@
## master #908 +/- ##
==========================================
+ Coverage 84.73% 87.35% +2.62%
==========================================
Files 169 169
Lines 2790 2791 +1
Branches 302 302
==========================================
+ Hits 2364 2438 +74
+ Misses 347 282 -65
+ Partials 79 71 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 looks like you might need to rebase or something? It's got a rather large number of unrelated formatting changes, possibly due to a different version of black? They're not wrong it just makes it a pain to review.
cve_bin_tool/cvedb.py
Outdated
@@ -91,7 +92,10 @@ async def nist_scrape(self, session): | |||
json_meta_links = self.META_REGEX.findall(page) | |||
return dict( | |||
await asyncio.gather( | |||
*[self.getmeta(session, meta_url) for meta_url in json_meta_links] | |||
*[ | |||
self.getmeta(session, self.META_LINK + meta_url) |
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.
Can we switch this to an f-string? We've stopped using + for string manipulation. More details here: https://github.com/intel/cve-bin-tool/blob/master/doc/CONTRIBUTORS.md#string-formatting
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.
But other than that, this looks like what we need. Let's just get the PR cleaned up a little bit so we can merge it without needing fixes. Thanks! 👍
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 revert back the changes after switching to f-string. I think this will work now
Also, totally late notice, but we're doing a meeting for pre-release and hacktoberfest preparation tomorrow at 9:30am US pacific (which would be 10pm in India assuming your github location is up to date). If you're interested and get this before we're done, we'll be on google meet: meet.google.com/dom-unde-bfk |
It turns out I am using the same version of black we are using in tests. The tests tend to fail if I use an older version of black. @terriko Should I create another PR just for the formatting changes? |
I think we should either do this or stick to a single version of black instead of using the GitHub 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.
Thank you! Not sure what's up with black, but thank you very much for persevering!
Closes #906