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 Homebrew Package support #44

Closed

Conversation

nk4456542
Copy link

Hello, I have added Homebrew support by looking at the previous code and the json API, but could not test locally because I did not know how to generate the homebrew.json json file so that I can pass it as the "expected result" in the test function.

Also I found the API of brew similar to rubygem's API, I have modified the code to match the brew json API format. Also while going through the brew json api docs https://formulae.brew.sh/docs/api/ , I found that the download link can be present in the "URL" or in the "URL"(see the above doc for the docker example) under another dict of "URLs" in the json api (the example of "a2ps" api mentioned in the issue). Please review the code that I have written, if further changes are required, do tell me , I will try to implement it. Also I am new to this repo, so I found no docs related to it (the commit msg doc or the overall working of the project), if there is any doc related to it, i would be happy to read and improve the project further. Thank you

@TG1999
Copy link
Collaborator

TG1999 commented Oct 11, 2020

Homebrew.json will be the expected output for a particular homebrew package, you can look at rubygems.json :)

@nk4456542
Copy link
Author

@TG1999, have made a test function and added homebrew.json and homebrew_mock_data.json for node package. The test seems to work fine locally here, do a review when you have time, I am a bit new to open source in general, any feedback or advice would be highly appreciated. I mean I have pushed to the same pull request, should I have created another pull request for this, or is it fine? a bit confused about this. Anyway, do a review when you are free.

@@ -91,7 +91,8 @@ def get_cargo_data_from_purl(purl):
)
versions = response.get("versions", [])
for version in versions:
version_purl = PackageURL(type=purl.type, name=name, version=version.get("num"))
version_purl = PackageURL(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't change these lines :), they are out of scope of this PR

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I have a setting of Format on save on my vscode, it happened unknowingly, will take care in the future. Thanks for the feedback.

base_path = "https://formulae.brew.sh/api/formula"
api_url = f"{base_path}/{name}.json"
response = get_response(api_url)
declared_license = response.get("license") or None
Copy link
Collaborator

@TG1999 TG1999 Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use or None

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will change it.

response = get_response(api_url)
declared_license = response.get("license") or None
homepage_url = response.get("homepage")
version = response.get("versions") or None
Copy link
Collaborator

@TG1999 TG1999 Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use or None

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will change it.

declared_license = response.get("license") or None
homepage_url = response.get("homepage")
version = response.get("versions") or None
download_url = response.get("urls")["stable"]["url"]
Copy link
Collaborator

@TG1999 TG1999 Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if response does not have "urls" ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add try and catch or would you suggest something else?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation of the homebrew json api tells that the download URL will be present inside a stable of URLs, refer here. In the docs, as they have mentioned, if we are using the base path as https://formulae.brew.sh/api/formula.json, then the download URL will be present inside the stable of urls, so I thought of leaving the download URL as such. Please do suggest what should I add it to cover the edge cases, so that it does not fail.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look how these things are done in the above block of code

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will look into it. Thanks for the feedback, will do the changes by tomorrow.

@nk4456542
Copy link
Author

@TG1999, have made the changes following your review, do take a look when you have time. I am really sorry for all those commits, I am having issues pushing and pulling and rebasing it. Will Improve it, please suggest some resource to learn remote git, I know how to handle local git, but don't know why anytime I push some changes, it asks me to pull the data in and then merge conflict occurs. Again sorry for all the commits, will squash it once all the reviewed changes are done. Thanks for your time reviewing it.

declared_license = response.get("license")
homepage_url = response.get("homepage")
version = response.get("versions")
download_path = response.get("urls")["stable"]["url"]
Copy link
Collaborator

@TG1999 TG1999 Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

urls = response.get('urls') or {{}}
stable_url = urls.get('stable') or {}
download_url = stable_url.get('url')

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove line 345-349 and use this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ok, will replace that, sorry for these mistakes, will remember and learn continuing further. Thank you for the feedback

response = get_response(api_url)
declared_license = response.get("license")
homepage_url = response.get("homepage")
version = response.get("versions")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extraction of version does not seem to be correct

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again sorry will correct this too.

@nk4456542
Copy link
Author

@TG1999, have made the changes:

  1. I used double quotes("") instead of single quotes('') in the code that you provided me to replace, so as to maintain the consistency and match the code that was written in the file. I hope that is not a problem
  2. I have not included a case where if the versions are not available ie, I have changed the below line for the versions extraction:
    version=response.get("versions")["stable"]. I did not know if I needed to add the case where the versions are not available in the API. If I should, do mention, will change it.
  3. Ran the tests locally, tests are passed successfully.
  4. I am really sorry for not writing code in a production manner, I am new to this, so thanks for helping out, hopefully, will get better at this once I get the hang of it. Again appreciate all the feedback.

@TG1999
Copy link
Collaborator

TG1999 commented Oct 14, 2020

Change the code for version

@nk4456542
Copy link
Author

Change the code for version

@TG1999, made the change, give it a look when you are free. :)

@nk4456542
Copy link
Author

@TG1999, just an update, have made the requested changes, please give a look when you are free.

Copy link
Contributor

@steven-esser steven-esser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nk4456542 Thanks for this contribution.

See my comment for one slight problem with the code.

Additionally, it would be nice to add a few more test cases to ensure your code if working as it should in all cases.

You can find numerous homebrew forumale examples here: https://formulae.brew.sh/

"type": "homebrew",
"namespace": null,
"name": "node",
"version": null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should collect version data; it seems like there is a bug in your code, as I see you attempt to collect this version data in package.py (on line 344-345)

Copy link
Author

@nk4456542 nk4456542 Oct 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaJuRG, Thanks for pointing out the bug, I know this is supposed to be a good first issue, but I am really new to the development side, I appreciate all the help.

But I am stuck here, as you said I changed the version in the tests/data/homebrew.json file. But I do not know how to pass the versions to the Package to yield it, I tried the key version, but did not work. So please tell me how to pass the version variable to the package.

Here is the diff of the test when I ran python -m pytest -vv

 Full diff:
E           [
E            {'api_data_url': None,
E             'api_url': 'https://formulae.brew.sh/api/formula/node.json',
E             'bug_tracking_url': None,
E             'code_view_url': None,
E             'contains_source_code': None,
E             'copyright': None,
E             'declared_license': 'MIT',
E             'dependencies': [],
E             'description': None,
E             'download_url': 'https://nodejs.org/dist/v14.13.1/node-v14.13.1.tar.gz',
E             'homepage_url': 'https://nodejs.org/',
E             'keywords': [],
E             'license_expression': None,
E             'md5': None,
E             'name': 'node',
E             'namespace': None,
E             'notice_text': None,
E             'parties': [],
E             'primary_language': None,
E             'purl': 'pkg:homebrew/node',
E         -   'qualifiers': {},
E         +   'qualifiers': OrderedDict(),
E             'release_date': None,
E             'repository_download_url': None,
E             'repository_homepage_url': None,
E             'root_path': None,
E             'sha1': None,
E             'sha256': None,
E             'sha512': None,
E             'size': None,
E             'source_packages': [],
E             'subpath': None,
E             'type': 'homebrew',
E             'vcs_url': None,
E         -   'version': '14.4.0'},
E         +   'version': None},
E           ]

I also tried to check the above code and see if I could implement it in a way it is implemented above, but did not understand that, I am really sorry for this.
I am really sorry to disturb you, appreciate all the help. Thank you

@nk4456542
Copy link
Author

@TG1999, could you please help me, I am stuck here: #44 (comment)

homepage_url = response.get("homepage")
versions = response.get("versions") or {}
version = versions.get("stable")
urls = response.get("urls") or {{}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{{}} cannot work and does not work.

>>> type({{}})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unhashable type: 'dict'

@pombredanne
Copy link
Contributor

@nk4456542 I am closing this for now for lack of response. Thank you for trying and and feel free to reopen if you want to finish this

@pombredanne pombredanne closed this Feb 9, 2022
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.

4 participants