Add providing location for fetch #56
Conversation
quepop
force-pushed
the
master
branch
4 times, most recently
from
3193c01
to
a6b3f6c
Compare
-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 <[email protected]>
-Testing filename deduction for http/https/ftp schemes Signed-off-by: Mateusz Perc <[email protected]>
| import os | ||
| import tempfile | ||
| from pathlib import Path | ||
| from pathlib import PurePosixPath |
There was a problem hiding this comment.
maybe just PurePath? It will be PurePosixPath on posix anyway and ci runs tests on {linux,macos,windows}
There was a problem hiding this comment.
I use PurePosixPaths only to handle URLs so i think it is more explicit that way.
| from urllib.parse import urlparse | ||
| from kiss_headers import parse_it |
There was a problem hiding this comment.
wrong import order https://www.python.org/dev/peps/pep-0008/#imports
| @@ -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 | |||
There was a problem hiding this comment.
no need to move this import, it was correctly placed: https://www.python.org/dev/peps/pep-0008/#imports
| 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) |
There was a problem hiding this comment.
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
| ], | ||
| ) | ||
| @mock.patch("fetchcode.requests.get") | ||
| def test_fetch_http_with_filename_deduction(mock_get, parameters, expected_filename): |
There was a problem hiding this comment.
consider changing parameters name to something more fitting, maybe content_disposition or filename?
| @pytest.mark.parametrize( | ||
| "parameters, expected_filename", | ||
| [ | ||
| pytest.param(f'filename*="{FILENAMES[0]}"; filename=""', FILENAMES[0]), |
There was a problem hiding this comment.
consider adding few test cases:
| 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
| 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: |
There was a problem hiding this comment.
why name it when the name is not used?
|
|
||
| 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: |
There was a problem hiding this comment.
Thats not my test (black reformatted it)
| 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: |
|
|
||
| FILENAMES = ["a.ext", "b.ext", "c.ext"] |
There was a problem hiding this comment.
consider switching to dict
| 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
-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 [email protected]