-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add support for homebrew packages #47
base: master
Are you sure you want to change the base?
Conversation
Please use "black" for formatting |
On the entire code?.... I thought that it might disturb the rest of code. |
Run black on the files that you are submitting for review |
Ok |
@TG1999 I have ran black on the files I changed. |
@mock.patch("fetchcode.package.get_response") | ||
def test_homebrew_packages(mock_get): | ||
side_effect = [file_data("tests/data/homebrew_mock_data.json")] | ||
purl = "pkg:homebrew/node" |
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.
Please make one more test case where purl has a version in it.
@TG1999 I have added additional test case. |
Hey @tushar912 please check what a version inside purl means, checkout packageurl-python to know more |
Oh I misunderstood , I would make the change ASAP. |
Signed-off-by: Tushar912 <[email protected]>
@pombredanne @MaJuRG ping |
versions = response.get("versions") or {} | ||
version_purl = PackageURL(type=purl.type, name=name, version=versions.get("stable")) | ||
yield Package( | ||
homepage_url=homepage_url, | ||
api_url=api_url, | ||
declared_license=declared_license, | ||
download_url=download_url, | ||
**version_purl.to_dict(), | ||
) |
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.
If there is more than one entry in versions
, wouldnt we need to loop thru them all?
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.
@MaJuRG I checked and from what I can see there is only one version of interest under the attribute "stable"
@tushar912 do you concur?
urls = response.get("urls") or {} | ||
stable_url = urls.get("stable") or {} | ||
download_url = stable_url.get("url") | ||
yield Package( |
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.
Why do we yield a package without a version and one with a version?
This is highly similar (of course) to the code in #44 .... when merging (it still needs love) I think it would be best to credit both authors. |
@tushar912 gentle ping ... do you still want to finish merge this so we can merge? If not we may close it soon. Thanks! |
Handle as_text correctly in cache
I have added support for homebrew packages.