Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tushar912
Copy link

I have added support for homebrew packages.

@TG1999
Copy link
Member

TG1999 commented Feb 11, 2021

Please use "black" for formatting

@tushar912
Copy link
Author

On the entire code?.... I thought that it might disturb the rest of code.

@TG1999
Copy link
Member

TG1999 commented Feb 11, 2021

Run black on the files that you are submitting for review

@tushar912
Copy link
Author

Ok

@tushar912
Copy link
Author

@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"
Copy link
Member

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.

@tushar912
Copy link
Author

@TG1999 I have added additional test case.

@TG1999
Copy link
Member

TG1999 commented Mar 1, 2021

Hey @tushar912 please check what a version inside purl means, checkout packageurl-python to know more

@tushar912
Copy link
Author

Oh I misunderstood , I would make the change ASAP.

@TG1999
Copy link
Member

TG1999 commented Mar 2, 2021

@pombredanne @MaJuRG ping

Comment on lines +354 to +362
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(),
)
Copy link
Contributor

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?

Copy link
Member

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(
Copy link
Member

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?

@pombredanne
Copy link
Member

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.

@pombredanne
Copy link
Member

@tushar912 gentle ping ... do you still want to finish merge this so we can merge? If not we may close it soon. Thanks!

pombredanne added a commit that referenced this pull request Feb 9, 2022
Handle as_text correctly in cache
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants