Add providing location for fetch #56

quepop · 2021-06-11T01:36:00Z

-extends #54
-Added location providing and filename deduction for every scheme
-Added a bunch of tests for the new functionality

Signed-off-by: Mateusz Perc m.perc@samsung.com

pombredanne

Thanks!
Do you think you could add a test?

-extends aboutcode-org#54 -Added filename deduction (content-disposition/URL) -Fetch and its helper functions now use pathlib's Path Signed-off-by: Mateusz Perc <m.perc@samsung.com>

-Testing filename deduction for http/https/ftp schemes Signed-off-by: Mateusz Perc <m.perc@samsung.com>

aalexanderr · 2021-08-20T10:33:55Z

src/fetchcode/__init__.py

-import os
-import tempfile
+from pathlib import Path
+from pathlib import PurePosixPath


maybe just PurePath? It will be PurePosixPath on posix anyway and ci runs tests on {linux,macos,windows}

I use PurePosixPaths only to handle URLs so i think it is more explicit that way.

aalexanderr · 2021-08-26T10:53:06Z

src/fetchcode/__init__.py

 from urllib.parse import urlparse
+from kiss_headers import parse_it


wrong import order https://www.python.org/dev/peps/pep-0008/#imports

aalexanderr · 2021-08-26T10:54:06Z

tests/test_fetch.py

@@ -14,52 +14,103 @@
 # CONDITIONS OF ANY KIND, either express or implied. See the License for the
 # specific language governing permissions and limitations under the License.

+from fetchcode import fetch


no need to move this import, it was correctly placed: https://www.python.org/dev/peps/pep-0008/#imports

aalexanderr · 2021-08-26T11:02:21Z

tests/test_fetch.py

+    mock_get.return_value.headers = {"content-disposition": f"attachment; {parameters}"}
+    with mock.patch("fetchcode.open", mock.mock_open()) as mocked_file:
+        location = Path.home()
+        response = fetch(HTTP_URL + FILENAMES[2], location)


FILENAMES[2] is confusing. I assume this is to test 3rd input set where name is inferred from url?
It could be made more readable - add url_filename parameter to tests and name those inputs (see comment above) so its clear what is tested here

aalexanderr · 2021-08-26T11:04:01Z

tests/test_fetch.py

+    ],
+)
+@mock.patch("fetchcode.requests.get")
+def test_fetch_http_with_filename_deduction(mock_get, parameters, expected_filename):


consider changing parameters name to something more fitting, maybe content_disposition or filename?

aalexanderr · 2021-08-26T11:04:51Z

tests/test_fetch.py

+@pytest.mark.parametrize(
+    "parameters, expected_filename",
+    [
+        pytest.param(f'filename*="{FILENAMES[0]}"; filename=""', FILENAMES[0]),


consider adding few test cases:

Suggested change

pytest.param(f'filename*="{FILENAMES[0]}"; filename=""', FILENAMES[0]),

pytest.param(f'filename*="{FILENAMES["a"]}"', FILENAMES["a"], id='should_choose_filename*'),

pytest.param(f'filename="{FILENAMES["a"]}"', FILENAMES["a"], id='should_choose_filename'),

pytest.param(f'filename*="{FILENAMES["a"]}"; filename={FILENAMES["b"]}', FILENAMES["a"], id='filename*_should_take_precedence'),

pytest.param(f'filename*="{FILENAMES["a"]}"; filename=""', FILENAMES["a"], id='should_choose_non_empty_filename*'),

pytest.param(f'filename*=""; filename="{FILENAMES["a"]}"', FILENAMES["a"], id='should_choose_non_empty_filename'),

this could be more readable if filename in url would also be param here and explicitly tested

aalexanderr · 2021-08-26T11:06:52Z

tests/test_fetch.py

+    location = Path.home()
+    mock_get.return_value.headers = {}
+    mock_tmp.return_value.name = location / FILENAMES[0]
+    with mock.patch("fetchcode.open", mock.mock_open()) as mocked_file:


why name it when the name is not used?

aalexanderr · 2021-08-26T11:08:27Z

tests/test_fetch.py

-
-    with mock.patch('fetchcode.open', mock.mock_open()) as mocked_file:
-        url = 'https://raw.githubusercontent.com/TG1999/converge/master/assets/Group%2022.png'
+    with mock.patch("fetchcode.open", mock.mock_open()) as mocked_file:


no need to name it

Thats not my test (black reformatted it)

aalexanderr · 2021-08-26T11:09:39Z

tests/test_fetch.py

        assert mock_ftp.retrbinary.called


+@mock.patch("fetchcode.FTP")
+def test_fetch_ftp_with_filename_deduction(mock_ftp):
+    with mock.patch("fetchcode.open", mock.mock_open()) as mocked_file:


aalexanderr · 2021-08-26T11:13:01Z

tests/test_fetch.py


+FILENAMES = ["a.ext", "b.ext", "c.ext"]


consider switching to dict

Suggested change

FILENAMES = ["a.ext", "b.ext", "c.ext"]

FILENAMES = {"a": "a.ext",

"b": "b.ext",

"c": "c.ext"}

so you can access them as FILENAMES["a"] making it a little bit clearer than positionals

Add black codestyle test for skeleton

quepop closed this Jun 11, 2021

quepop reopened this Jun 11, 2021

pombredanne requested changes Jun 11, 2021

View reviewed changes

quepop marked this pull request as draft June 14, 2021 00:37

quepop force-pushed the master branch 4 times, most recently from 3193c01 to a6b3f6c Compare June 24, 2021 00:20

quepop marked this pull request as ready for review June 24, 2021 00:43

quepop requested a review from pombredanne June 25, 2021 17:21

quepop added 2 commits June 25, 2021 21:05

Add providing location for fetch

640e874

-extends aboutcode-org#54 -Added filename deduction (content-disposition/URL) -Fetch and its helper functions now use pathlib's Path Signed-off-by: Mateusz Perc <m.perc@samsung.com>

Add tests for location providing

ac2461b

-Testing filename deduction for http/https/ftp schemes Signed-off-by: Mateusz Perc <m.perc@samsung.com>

quepop force-pushed the master branch from f1ab1aa to ac2461b Compare June 25, 2021 19:06

quepop mentioned this pull request Aug 5, 2021

Add AlpinePackages pipeline aboutcode-org/scancode.io#272

Closed

aalexanderr suggested changes Aug 26, 2021

View reviewed changes

aalexanderr mentioned this pull request Oct 4, 2021

Consider switching to curl / wget for http(s) ftp #72

Open

pombredanne added a commit that referenced this pull request May 10, 2022

Merge pull request #56 from keshav-space/main

c7084ee

Add black codestyle test for skeleton

keshav-space marked this pull request as draft September 19, 2024 09:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add providing location for fetch #56

Add providing location for fetch #56

quepop commented Jun 11, 2021 •

edited

Loading

pombredanne left a comment

aalexanderr Aug 20, 2021

quepop Aug 27, 2021 •

edited

Loading

aalexanderr Aug 26, 2021

aalexanderr Aug 26, 2021

aalexanderr Aug 26, 2021

aalexanderr Aug 26, 2021

aalexanderr Aug 26, 2021

aalexanderr Aug 26, 2021

aalexanderr Aug 26, 2021

quepop Aug 27, 2021

aalexanderr Aug 26, 2021

aalexanderr Aug 26, 2021

		from urllib.parse import urlparse
		from kiss_headers import parse_it

-        pytest.param(f'filename*="{FILENAMES[0]}"; filename=""', FILENAMES[0]),
+        pytest.param(f'filename*="{FILENAMES["a"]}"', FILENAMES["a"], id='should_choose_filename*'),
+        pytest.param(f'filename="{FILENAMES["a"]}"', FILENAMES["a"], id='should_choose_filename'),
+        pytest.param(f'filename*="{FILENAMES["a"]}"; filename={FILENAMES["b"]}', FILENAMES["a"], id='filename*_should_take_precedence'),
+        pytest.param(f'filename*="{FILENAMES["a"]}"; filename=""', FILENAMES["a"], id='should_choose_non_empty_filename*'),
+        pytest.param(f'filename*=""; filename="{FILENAMES["a"]}"', FILENAMES["a"], id='should_choose_non_empty_filename'),

-FILENAMES = ["a.ext", "b.ext", "c.ext"]
+FILENAMES = {"a": "a.ext",
+                         "b": "b.ext",
+                         "c":  "c.ext"}

Add providing location for fetch #56

Are you sure you want to change the base?

Add providing location for fetch #56

Conversation

quepop commented Jun 11, 2021 • edited Loading

pombredanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quepop Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quepop commented Jun 11, 2021 •

edited

Loading

quepop Aug 27, 2021 •

edited

Loading