Add support for downloading http and ftp urls by TG1999 · Pull Request #22 · aboutcode-org/fetchcode

TG1999 · 2020-05-12T16:37:58Z

I have modified the existing API according to issue #20 and #21, test cases are pending I will try to update them by tommorow.
Signed-off-by: TG1999 tushar.goel.dav@gmail.com

TG1999 · 2020-05-13T05:32:49Z

@MaJuRG how it looks initially, please review it :D

steven-esser · 2020-05-13T20:34:09Z

+

-def fetch(url):
+def fetch(url, filename=None):


What is the purpose of filename?

If user wants to save the file at a customised location, then can specify the location here.

Ok, I would change this variable name to something more descriptive, like location or download_path or something similar.

Agreed, changing the name

@MaJuRG , I have changed the name

@MaJuRG can you please also tell what is left in this PR :D

steven-esser · 2020-06-01T18:12:55Z

+    try:
+        r = urlopen(url)
+
+    except:


Can you except the exact Exception here? This way we know if some other unknown exception is raised, instead of catching everything.

Okay got this

steven-esser · 2020-06-01T18:14:09Z

+    if 'Content-Type' in info:
+        content_type = info['Content-Type']
+
+    if 'Content-length' in info:
+        size = int(info['Content-length'])


Couldnt these be replaced with something like content_type = info.get('Content-Type') instead of using if statements.

Got this one

I have added another if for handling type of size, it will be None if its not found else it's needed to be converted into int

@MaJuRG review it please :D

pombredanne

Thank you... Please see my comments inline.

pombredanne · 2020-06-16T08:37:24Z

+import shutil
 import tempfile 
-
-import requests


Are you really dropping using requests?

Yes, I thought I should use urllib since it can support both http and ftp urls

hum... I am not sure that the convenience is worth the possible trouble. "requests" is a tad more robust than the plain urllib AFAIK. Also have you considered https://pypi.org/project/ftputil/ also for FTP?

what about we use ftplib ?

pombredanne · 2020-06-16T08:37:46Z

 - `content_type`: content type of the file
 - `size`: size of the retrieved content in bytes
 - `url`: fetched URL
+- `scheme` : scheme of the URL


Why return the scheme too?

What I thought thay since earlier our code was only for http based URLs, since we are also supporting ftp based URLs, so I should also save the scheme, so what's your thoughts over it? Should we save it or not :)

It is already in the URL, so what will you do with the returned value?

Okay got this, making changes for this :D

pombredanne · 2020-06-16T08:38:57Z

+

-def fetch(url):
+def fetch(url, location=None):


What is location about?

If user wants to give a customised location of its own choice, while using this library, they can use this location parameter.

OK, then explain that in the function docstring

Got this one

pombredanne · 2020-06-16T13:50:18Z

+    url_parts = urlparse(url)
+
+    scheme = url_parts.scheme
+


Why the blank line? Does it help with readability here?

Okay got this one,

pombredanne · 2020-06-16T13:52:56Z

@@ -14,19 +14,148 @@
 # CONDITIONS OF ANY KIND, either express or implied. See the License for the


Please tiny test files that are just a few bytes. https://github.com/nexB/fetchcode/blob/4ad6ca4384e789e4b3cff5e1daf28a927f07be7f/tests/data/img.png is way too big we like to try to avoid polluting the version control history with big test files when possible.

Furthermore it may a copyrighted image?

Okay got this one

pombredanne · 2020-06-16T13:53:36Z

-@mock.patch('fetchcode.api.requests.get')
-def test_fetch(mock_get):
+
+@mock.patch('fetchcode.api.urlopen')


Do we really need an "api" module?

Can you elaborate a little 😅

may be you could use a flatter namespace and have your api functions attached directly under fetchcode.init.py?

pombredanne

Thanks!
See some comments inilne.

pombredanne · 2020-06-20T15:33:06Z

+- `location`: 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


Indent by 4 spaces ... but you likely would want these to be in the init docstring

pombredanne · 2020-06-20T15:34:47Z

+    """
+
+    def __init__(self, location, content_type, size, url):
+        self.location = location


What is the most salient attribute of a Response? IMHO you should order the attributes as: url, size, content_type, location

pombredanne · 2020-06-20T15:36:39Z

+def fetch_via_http(url, location):
+    """
+    Return a `Response` object built from fetching the content at a HTTP/HTTPS based `url` URL string
+    at the `location` location string   


For "at the location location string"
what about instead:
"saving the content in a file at location"
?

Makes sense to me too

pombredanne · 2020-06-20T15:37:23Z

+    size = ftp.size(path)
+    mime = MimeTypes()
+    mime_type = mime.guess_type(file)
+    content_type = mime_type[0]


What if no content_type is returned?

pombredanne · 2020-06-20T15:40:29Z

+    """
+    Return a `Response` object built from fetching the content at the `url` URL string.
+    Take `location` as an optional parameter and store content at that location if specified,
+    else use tempfile.


else use tempfile. --> otherwise create and return a temporary file may be better?

Makes sense to me too

pombredanne · 2020-06-20T15:41:37Z

+    Take `location` as an optional parameter and store content at that location if specified,
+    else use tempfile.
+    """
+    if location == None:


Use instead if not location:
But is it really a worthy convenience to have that optional feature?

I need your suggestion over this, what are your thoughts over this

pombredanne · 2020-06-20T15:42:45Z

+
+    fetcher = {"ftp": fetch_via_ftp, "http": fetch_via_http, "https": fetch_via_http}
+
+    return fetcher.get(scheme)(url, location)


What if fetcher.get(scheme) returns None?

pombredanne · 2020-06-20T15:43:28Z

-from fetchcode import api
-
-@mock.patch('fetchcode.api.requests.get')
-def test_fetch(mock_get):


Did you keep the tests somehow?

These all are deleted :p

pombredanne · 2020-06-20T15:44:04Z

-        assert hasattr(response,'size')
-        assert hasattr(response,'location')
-        assert hasattr(response,'url')
-        assert hasattr(response,'content_type') 


Note that rather than only checking the presence of an attribute, check its value

This is also deleted

test_fetch is the new file now where tests reside regarding this code

TG1999 · 2020-06-22T16:15:59Z

@pombredanne please review the changes :)

steven-esser · 2020-06-22T23:55:57Z

+    size = ftp.size(path)
+    mime = MimeTypes()
+    mime_type = mime.guess_type(file)
+    content_type = mime_type[0] if mime_type and len(mime_type) >= 1 else None


This is not very readable.

Got this one

steven-esser · 2020-06-22T23:56:31Z

+        self.location = location
+
+
+def fetch_via_http(url, location):


Lets remove the via string from all these methods.

Okay got this !!

steven-esser · 2020-06-22T23:56:53Z

+    else:
+        return


This is redundant.

Oops, making it right, Thanks :)

steven-esser

This is looking OK. From what I can see, we are not getting the actual content that these URLs are pointing to. Is this by design?

Perhaps we do not want this content, but I had thought we did need it. @pombredanne ping

pombredanne

Thank you!
See some comments inline.

pombredanne · 2020-07-01T10:02:32Z

@@ -13,3 +13,16 @@ Then install all the requirements using
 To run test suite


Please RestructuredText (.rst) rather than Markdown for docs.

pombredanne · 2020-07-01T10:05:07Z

+
+def test_fetch_with_scheme_not_present():
+    url = "abc://speedtest/1KB.zip"
+    response = fetch(url=url)


IMHO this should raise an exception when there is an unsupported/unknown scheme.

pombredanne · 2020-07-01T10:06:27Z

+    url_parts = urlparse(url)
+    scheme = url_parts.scheme
+
+    fetcher = {"ftp": fetch_ftp, "http": fetch_http, "https": fetch_http}


that's a collection, so the variable name should be plural

pombredanne · 2020-07-01T10:07:59Z

+
+    fetcher = {"ftp": fetch_ftp, "http": fetch_http, "https": fetch_http}
+
+    if scheme in fetcher:


When the scheme is not there we should have an exception and not return None.

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

steven-esser

Looks good to me.

@pombredanne Anything to add here?

pombredanne

All good now. Thank you ++
Merging!

TG1999 marked this pull request as draft May 12, 2020 18:33

TG1999 marked this pull request as ready for review May 13, 2020 05:29

steven-esser reviewed May 13, 2020

View reviewed changes

TG1999 linked an issue May 15, 2020 that may be closed by this pull request

Download from ftp:// URLs #21

Open

TG1999 removed a link to an issue May 15, 2020

Download from ftp:// URLs #21

Open

steven-esser suggested changes Jun 1, 2020

View reviewed changes

steven-esser approved these changes Jun 8, 2020

View reviewed changes

pombredanne requested changes Jun 16, 2020

View reviewed changes

pombredanne reviewed Jun 16, 2020

View reviewed changes

pombredanne requested changes Jun 20, 2020

View reviewed changes

TG1999 requested a review from pombredanne June 22, 2020 16:15

steven-esser suggested changes Jun 22, 2020

View reviewed changes

steven-esser approved these changes Jun 23, 2020

View reviewed changes

Comment thread fetchcode/__init__.py

steven-esser suggested changes Jun 29, 2020

View reviewed changes

Comment thread fetchcode/__init__.py

pombredanne requested changes Jul 1, 2020

View reviewed changes

Add support HTTP/s and FTP URLs

1a7ee55

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

steven-esser suggested changes Jul 7, 2020

View reviewed changes

Comment thread fetchcode/__init__.py

steven-esser approved these changes Jul 21, 2020

View reviewed changes

TG1999 changed the title ~~[WIP] Add support for downloading http and ftp urls~~ Add support for downloading http and ftp urls Jul 22, 2020

pombredanne approved these changes Jul 22, 2020

View reviewed changes

pombredanne merged commit 85f46ba into aboutcode-org:master Jul 22, 2020

		@@ -14,19 +14,148 @@
		# CONDITIONS OF ANY KIND, either express or implied. See the License for the


		fetcher = {"ftp": fetch_via_ftp, "http": fetch_via_http, "https": fetch_via_http}

		return fetcher.get(scheme)(url, location)

		@@ -13,3 +13,16 @@ Then install all the requirements using
		To run test suite


		fetcher = {"ftp": fetch_ftp, "http": fetch_http, "https": fetch_http}

		if scheme in fetcher:

Uh oh!

Uh oh!

Conversation

TG1999 commented May 12, 2020

Uh oh!

TG1999 commented May 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steven-esser May 14, 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!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

steven-esser May 14, 2020 •

edited

Loading