Add tag support for GitHub URLs by TG1999 · Pull Request #12 · aboutcode-org/fetchcode

TG1999 · 2020-03-09T12:10:21Z

Solves issue #6 for GitHub URLs
Signed-off-by: TG1999 tushar.goel.dav@gmail.com

TG1999 · 2020-03-09T12:13:14Z

@pombredanne @MaJuRG, please check this PR :)

TG1999 · 2020-03-09T12:23:30Z

I ran it for https://github.com/nexb/scancode-toolkit and got this
['v3.1.1', 'v3.1.0', 'v3.0.2', 'v3.0.1', 'v3.0.0', 'v2.9.9', 'v2.9.8', 'v2.9.7', 'v2.9.6', 'v2.9.5', 'v2.9.4', 'v2.9.3', 'v2.9.2', 'v2.9.1', 'v2.9.0b1', 'v2.2.1', 'v2.2.0', 'v2.1.0', 'v2.0.1', 'v2.0.0', 'v2.0.0.rc3', 'v2.0.0.rc2', 'v2.0.0.rc1', 'v1.6.3', 'v1.6.2', 'v1.6.1', 'v1.6.0', 'v1.5.0', 'v1.4.3', 'v1.4.2']
so I thik it's good to go

steven-esser

You need to add more tests here. Convert the test you did above locally into an actual test case as a first step.

TG1999 · 2020-03-09T16:38:58Z

Okay should I match both the arrays @MaJuRG the one that I shown in comments with the test case?

steven-esser · 2020-03-09T16:40:11Z

@TG1999 Assuming the list does not change over time, this is fine. Otherwise you can simply check for the presence of a few tag values that we know will continue to exists.

TG1999 · 2020-03-09T16:41:07Z

Okay I think I can match the last element of the array, since it won't change over time

TG1999 · 2020-03-09T16:57:22Z

@MaJuRG done the changes as per your comments :)

steven-esser · 2020-03-09T18:06:24Z

+def test_github_tags_for_not_empty_list():
+    url = 'https://github.com/nexb/scancode-toolkit'
+    response = github_tags.get_tags(url=url)
+    assert (response[-1] == 'v1.4.2') 


You should change this to assert 'v1.4.2' in response. This is easier to read.

Additionally, we should test for two additional items:

Another version number that is a valid tag in response

A version number that IS NOT in response

Done @MaJuRG :)

steven-esser

Are you accessing the internet in your test cases? To me, it seems you are (which is something we do not want to do). Just want to make sure

TG1999 · 2020-03-10T14:50:33Z

Yes, I am since using it since I have to acess GitHub api

steven-esser

We need to mock these API requests then. We do not want tests that rely on accessing potentially changing data on the web.

TG1999 · 2020-03-10T15:34:14Z

Cool, pushing changes in a while

TG1999 · 2020-03-10T15:35:54Z

I have mocked the responses now you can merge it :)

steven-esser · 2020-03-10T17:31:41Z

+@mock.patch('fetchcode.github_tags.requests.get')
+def test_github_tags_for_not_empty_list(mock_get):
+    url = 'https://github.com/nexb/scancode-toolkit'
+    mock_get.return_value.json.return_value =[


Can you move this big thing to a separate json file?

Cool, will do so

Done @MaJuRG :D

steven-esser

Since we are going to (probably) have multiple test files in the future, please:

Create a new directory tests/data/
Rename this JSON file to gh-scancode-tags-response.json or similar.

TG1999 · 2020-03-10T17:57:13Z

Okay making changes in 5 minutes

TG1999 · 2020-03-10T18:00:27Z

Done @MaJuRG :)

pombredanne

Thanks!
See some of my comments inline

pombredanne · 2020-03-11T01:07:03Z

+    """
+    Builds URL according to GitHub API 
+    """
+    return base_url + path + '/tags'


Can you describe what base_url and path are ? what would examples of values? Also using + sounds rather britlle...
What about using format or a f string instead? and does base_url ever changes?

And also is this function really warranted since you are not even testing it?

I have tested the get_tags() function that is using this function so should I also write unit test for this function 😅

pombredanne · 2020-03-11T01:07:13Z

+
+def build_url(base_url, path):
+    """
+    Builds URL according to GitHub API 


Do you mind using imperative style like in commit message?
e.g. Build URL according to GitHub API

or rather:
Return a URL to access the list of tags using the GitHub API

Okay got it

pombredanne · 2020-03-11T01:09:34Z

+
+def get_tags(url):
+    """
+    Takes Github VCS URL as input and provides 


Same comment as above: use imperative instead.

Return a list of git tags given the `url` to a Github repository

Okay got this

pombredanne · 2020-03-11T01:09:57Z

+        print('Not a GitHub URL')
+
+    else:
+        final_url = build_url(base_url=base_url, path=url_parts.path)


See my comments on build_url

pombredanne · 2020-03-11T01:10:18Z

+    else:
+        final_url = build_url(base_url=base_url, path=url_parts.path)
+        resp = requests.get(final_url)
+        print(resp)


Why do you print things?

Okay got this

pombredanne · 2020-03-11T01:10:51Z

+        print(resp)
+        tags = []
+        for res in resp.json():
+            tags.append(res['name'])


Are you sure 'name' is always there?

pombredanne

Thank you for the updates! see my comments

pombredanne · 2020-03-13T01:01:16Z

+    url_parts = urlparse(url)
+
+    if not(url_parts.netloc == 'github.com'):
+        print('Not a GitHub URL')


Why print anything? You do not print things in a library: you return values or you raise Exceptions.

Got this :)

pombredanne · 2020-03-13T01:03:52Z

+# under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
+# CONDITIONS OF ANY KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations under the License.
+from unittest import mock


Please organize your imports:

import xxx <-std lib imports, one per line, sorted empty line import zzz <-third-party imports, one per line, sorted empty line import foo <-own modules imports, one per line, sorted empty line empty line

Got this :), will take care for future too

pombredanne · 2020-03-13T01:04:28Z

+@mock.patch('fetchcode.github_tags.requests.get')
+def test_github_tags_for_not_empty_list(mock_get):
+    url = 'https://github.com/nexb/scancode-toolkit'
+    with open('tests/data/gh_tags_response.json','r') as file:


Why r mode? this is the default.

Got this :)

pombredanne · 2020-03-13T01:05:03Z

+        data = file.read()
+    mock_get.return_value.json.return_value = json.loads(data)
+    response = github_tags.get_tags(url=url)
+    assert 'v1.4.2' in response 


Why assert only three values and not everything?

Got this :)

pombredanne · 2020-03-13T01:05:39Z

+
+def test_build_url_with_conventional_URL():
+    url = 'https://github.com/nexb/fetchcode'
+    supposed_build_url = 'https://api.github.com/repos/nexb/fetchcode/tags'


supposed would likely be best as "expected" ?

Got this :)

pombredanne · 2020-03-13T01:07:18Z

+    url = 'https://github.com/nexb/fetchcode'
+    supposed_build_url = 'https://api.github.com/repos/nexb/fetchcode/tags'
+    final_url = github_tags.build_url(url)
+    assert final_url == supposed_build_url 


We typically write asserts the other ways as assert "expected value" == "obtained value"...
This ensures that in diffs the expected values are before or on the left and the actual obtained values are on the right or below... which is helpful as a convention when there are failure traces.

Will remember this for future ref. too

steven-esser · 2020-03-13T16:49:40Z

+    assert 'v3.1.1' in response
+    assert 'v3.1.0' in response
+    assert 'v3.0.2' in response
+    assert 'v3.0.1' in response
+    assert 'v3.0.0' in response
+    assert 'v2.9.9' in response
+    assert 'v2.9.8' in response
+    assert 'v2.9.7' in response
+    assert 'v2.9.6' in response
+    assert 'v2.9.5' in response
+    assert 'v2.9.4' in response
+    assert 'v2.9.3' in response
+    assert 'v2.9.2' in response
+    assert 'v2.9.1' in response
+    assert 'v2.9.0b1' in response
+    assert 'v2.2.1' in response
+    assert 'v2.2.0' in response
+    assert 'v2.1.0' in response
+    assert 'v2.0.1' in response
+    assert 'v2.0.0' in response
+    assert 'v2.0.0.rc3' in response
+    assert 'v2.0.0.rc2' in response
+    assert 'v2.0.0.rc1' in response
+    assert 'v1.6.3' in response
+    assert 'v1.6.2' in response
+    assert 'v1.6.1' in response
+    assert 'v1.6.0' in response
+    assert 'v1.5.0' in response
+    assert 'v1.4.3' in response
+    assert 'v1.4.2' in response
+    assert 'v5.0.1' not in response


Can you add these values to a single list and iterate thru them? There is no need to take up many lines here.

Okay cool, doing changes in 10 minutes

TG1999 · 2020-03-13T17:02:09Z

Done @MaJuRG 😄

TG1999 · 2020-03-19T20:34:28Z

Thanks @MaJuRG now waiting for @pombredanne

Signed-off-by: TG1999 <tushar.goel.dav@gmail.com>

pombredanne · 2020-08-20T13:18:59Z

Thanks! all merged.

steven-esser suggested changes Mar 9, 2020

View reviewed changes

steven-esser approved these changes Mar 9, 2020

View reviewed changes

steven-esser reviewed Mar 10, 2020

View reviewed changes

steven-esser suggested changes Mar 10, 2020

View reviewed changes

steven-esser approved these changes Mar 10, 2020

View reviewed changes

pombredanne requested changes Mar 11, 2020

View reviewed changes

pombredanne requested changes Mar 13, 2020

View reviewed changes

steven-esser suggested changes Mar 13, 2020

View reviewed changes

steven-esser approved these changes Mar 19, 2020

View reviewed changes

TG1999 requested a review from pombredanne March 27, 2020 15:33

Add tag support for GitHub URLs

83091f8

Signed-off-by: TG1999 <tushar.goel.dav@gmail.com>

pombredanne merged commit 451f648 into aboutcode-org:master Aug 20, 2020

Uh oh!

Uh oh!

Conversation

TG1999 commented Mar 9, 2020

Uh oh!

TG1999 commented Mar 9, 2020

Uh oh!

TG1999 commented Mar 9, 2020

Uh oh!

steven-esser left a comment

Choose a reason for hiding this comment

Uh oh!

TG1999 commented Mar 9, 2020

Uh oh!

steven-esser commented Mar 9, 2020

Uh oh!

TG1999 commented Mar 9, 2020

Uh oh!

TG1999 commented Mar 9, 2020

Uh oh!

steven-esser Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steven-esser left a comment

Choose a reason for hiding this comment

Uh oh!

TG1999 commented Mar 10, 2020

Uh oh!

steven-esser left a comment

Choose a reason for hiding this comment

Uh oh!

TG1999 commented Mar 10, 2020

Uh oh!

TG1999 commented Mar 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steven-esser left a comment

Choose a reason for hiding this comment

Uh oh!

TG1999 commented Mar 10, 2020

Uh oh!

TG1999 commented Mar 10, 2020

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

steven-esser Mar 9, 2020 •

edited

Loading