Design initial API by TG1999 · Pull Request #4 · aboutcode-org/fetchcode

TG1999 · 2019-10-08T07:27:04Z

Solves issue #3
Signed-off-by: tushar goel tushar.goel.dav@gmail.com

TG1999 · 2019-10-08T15:03:56Z

@pombredanne please have a look on it and suggest me some changes.

pombredanne

Thank you. See my comments inline.
There are some comments on the format, other on the code (which I reckon is really a placeholder) and some on the API. The API of the returned data could either be an object or a dict. I think an object would be better.

pombredanne · 2019-10-10T15:26:15Z

+    con = requests.head(url)
+    r = requests.get(url)
+    name = url.split('/')[-1]
+    filename = '../download/'+name


You cannot assume some location like this. It has to be some temp file instead.

I am not able to get this, like what should I have here ?

pombredanne · 2019-10-10T15:29:47Z

+        'type_of_data' : con.headers['content-type'],
+        'size' : con.headers['content-length'],
+        'url' : url,
+        'checksum' : hashlib.md5(open(filename,'rb').read()).hexdigest()


what if this fails? what if the file is not closed? And if the content is already in memory, why re-reading from disk?
Also why privilege md5 and make it the one value for checksum?

Okay please can you suggest me some alternative for this

pombredanne · 2019-10-10T15:31:34Z

+import requests 
+import hashlib
+def fetch(url):
+    con = requests.head(url)


Why to do issue a head first?

I am not able to get this please can you elaborate a little 😅

TG1999 · 2019-10-12T06:42:09Z

I have made some changes that you suggested, and I was not able to understand some suggestions please explain them again to me so I can complete them too 😀

pombredanne

Thanks for the updates... See my comments inline with the code

TG1999 · 2019-10-18T09:19:10Z

@pombredanne I have made some more changes I hope now the PR is good to merge 😀

pombredanne

Thank you: I posted several comments for your consideration. At a high level we want some detailed docstrings for API functions. And we should NOT catch exceptions and surely not print things on errors: this is meant to be a library, and printing error message will pollute the terminal of the users of the apps that will use this lib....AND they will not be able to do ay thing about it. (side note: try >>> import this ;) )
We likely need to have some tests too sooner than later.

pombredanne

Thank you. This is starting to look quite good. One thing thouh is that we do not want tests to make live network call. Instead you should use some mock object and never do a never call during a test

TG1999 · 2019-10-29T02:27:37Z

Thanks @pombredanne please can you give me some example, I am not able to understand which one should I use here 😅

TG1999 · 2019-11-02T13:52:28Z

@pombredanne done with Mocking of tests too, can the PR be merged now ?

pombredanne

Thank you for the updates... we are making good progress!

There are several small things to fix for your review... also you are missing tests for the fetch() function.

Now you also need to craft proper commit messages that are informative and detailed:

some fixes
some more fixes
etc are not great: what information does it carry? close to none. Say you review this commit streams in a few months from now: will these commit messages help you understand what the commits are about? I very much doubt so ;)

Good commit matter a lot: please see https://github.com/nexB/aboutcode/wiki/Writing-good-commit-messages and the related docs in details!
Thank you!

pombredanne · 2019-11-02T15:09:44Z

+        self.url = url
+
+
+def get_request(url):


What's the purpose of this function that is only a one liner? Is this to help with mock testing? if not it would be best inlined.

pombredanne · 2019-11-02T15:11:02Z

+    return requests.get(url)
+
+
+def Filewriter(filename,content):


We never use Title case for function names. Also always use a space between args.
That said, why a function for a simple write? IMHO it would best inlined, unless you can explain a good reason why,

pombredanne · 2019-11-02T15:12:46Z

+    """
+    Takes URL as parameter and write down the content inside it
+    to a temporary location and return a `Response` kind of object
+    """


I would prefer a formulation such as:

Return a `Response` object built from fetching the content at the `url` URL string.

It seems cool, doing it ;)

pombredanne · 2019-11-02T15:13:37Z

+    Takes URL as parameter and write down the content inside it
+    to a temporary location and return a `Response` kind of object
+    """
+    r = get_request(url=url)


inline this as explained above, unless you can explain why you need this

pombredanne · 2019-11-02T15:16:43Z

+import os
+from unittest.mock import Mock,patch,mock_open
+
+from nose.tools import assert_is_not_none


Please do not use nose. We prefer using pytest as a test runner and using simple plain assert instead.

Okay doing it

pombredanne · 2019-11-02T15:18:21Z

+
+from nose.tools import assert_is_not_none
+
+from src import api


if our module is called src, there is something wrong: there should be a a fetchcode module with an api submodule instead

Okay doing it.

pombredanne · 2019-11-02T15:20:18Z

+def test_get_request():
+    """
+    To test api when an image is being downloaded
+    """


Try to avoid docstring on test functions: instead using an explicit verbose function name is enough and clearer in most cases. It also helps with selecting tests.

pombredanne · 2019-11-02T15:21:46Z

+        mock_get.return_value.ok = True
+        url = 'https://raw.githubusercontent.com/lk-geimfari/unittest/master/logo.png'
+        response = api.get_request(url)
+    assert_is_not_none(response)


Use assert response is not None instead and do not use nose. That said, I am not sure what you are testing in this cases: can you explain a bit?

TG1999 · 2019-11-02T21:07:20Z

@pombredanne made all the changes and squashed all commits into one, I think the commit message is clean enough to describe the work

pombredanne

Thank you! there are only a few minor format comments to address and a suggested improvement on doc and field names... and we are good to go!

pombredanne · 2019-11-07T08:28:26Z

+    2) Content type of the file
+    3) Size of the file
+    4) URL of the file
+    """


What about using this instead:

Represent the response from fetching a URL with: - `filename`: the absolute location of the files that was fetched - `content_type`: content type of the file - `size`: size of the retrieved content in bytes - `url`: fetched URL

Also in hindsight, what about using location and not filename and ensure this is an absolute file location? and not just a file name?

All changes have been done, please have a look

Signed-off-by: tushar goel <tushar.goel.dav@gmail.com> some fixes Signed-off-by: tushar goel <tushar.goel.dav@gmail.com> some fixes Signed-off-by: tushar goel <tushar.goel.dav@gmail.com> some changes refined Signed-off-by: tushar goel <tushar.goel.dav@gmail.com> make some fixes Signed-off-by: tushar goel <tushar.goel.dav@gmail.com> Update api.py add tests and improve api Signed-off-by: tushar goel <tushar.goel.dav@gmail.com> Divide code into functions and test file writing and get requests with Mock Signed-off-by: tushar goel <tushar.goel.dav@gmail.com> Divide code Signed-off-by: tushar goel <tushar.goel.dav@gmail.com> test the functions inside API Signed-off-by: tushar goel <tushar.goel.dav@gmail.com> Design Initial API and test it with mock Object Signed-off-by: tushar goel <tushar.goel.dav@gmail.com> Nitpicking: Remove empty lines Nitpicking: change parameters name Design Initial API Signed-off-by: tushar goel <tushar.goel.dav@gmail.com>

TG1999 · 2019-11-09T12:37:06Z

@pombredanne please review it 😄 all the problems has been resolved I think it can be merged now

pombredanne

All good! Thank you!

pombredanne · 2019-11-11T14:29:14Z

+    2) Content type of the file
+    3) Size of the file
+    4) URL of the file
+    """


Signed-off-by: Ayan Sinha Mahapatra <ayansmahapatra@gmail.com>

pombredanne requested changes Oct 10, 2019

View reviewed changes

pombredanne changed the title ~~design initial API~~ Design initial API Oct 14, 2019

pombredanne requested changes Oct 14, 2019

View reviewed changes

Comment thread src/api.py Outdated

Comment thread src/api.py Outdated

Comment thread src/api.py Outdated

Comment thread src/api.py Outdated

Comment thread src/api.py Outdated

Comment thread src/api.py Outdated

pombredanne requested changes Oct 23, 2019

View reviewed changes

pombredanne requested changes Oct 28, 2019

View reviewed changes

pombredanne requested changes Nov 2, 2019

View reviewed changes

pombredanne requested changes Nov 7, 2019

View reviewed changes

pombredanne approved these changes Nov 11, 2019

View reviewed changes

Comment thread fetchcode/api.py

2) Content type of the file

3) Size of the file

4) URL of the file

"""

pombredanne Nov 11, 2019

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

👍

pombredanne merged commit 3c87ef1 into aboutcode-org:master Nov 11, 2019

pombredanne mentioned this pull request Nov 11, 2019

Design initial API #3

Closed

TG1999 mentioned this pull request May 6, 2020

Improve current API by adding more tests #19

Closed

JonoYang pushed a commit that referenced this pull request Aug 2, 2021

Add basic RTD documentaion #4

f2c1400

Signed-off-by: Ayan Sinha Mahapatra <ayansmahapatra@gmail.com>


		from nose.tools import assert_is_not_none

		from src import api

Uh oh!

Uh oh!

Conversation

TG1999 commented Oct 8, 2019

Uh oh!

TG1999 commented Oct 8, 2019

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TG1999 commented Oct 12, 2019

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TG1999 commented Oct 18, 2019

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

TG1999 commented Oct 29, 2019

Uh oh!

TG1999 commented Nov 2, 2019

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

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!