Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

additions to download_from_url and extract_archive #602

Merged
merged 8 commits into from
Oct 2, 2019
Merged

additions to download_from_url and extract_archive #602

merged 8 commits into from
Oct 2, 2019

Conversation

bentrevett
Copy link
Contributor

download_from_url would always fail to download from a non-google drive link due to a KeyError: 'content-disposition' from this line. This was fixed by explicitly getting the filename from the url instead of the url header.

This fixed caused some issues where google drive links thought end of the url was the filename, so this is handled by setting the filename back to None after checking if the url is or isn't a google drive link.

I also added support for .zip files in extract_archive. This involved removing the archive argument and having the function get it from the end of the filename. Had a quick look but this didn't seem to break any functionality.

Also, the download_extract.py file did not seem to work in the first place. It would try and do an os.path.split(path) on validation.tar.gz and then root would just be an empty string which would cause an error, changing the default to ./validation.tar.gz seems to fix that, but I am not 100% sure this is the intended functionality.

Copy link
Contributor

@zhangguanheng66 zhangguanheng66 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for contribution. Could you add unit tests to archive both tar and zip files. The lint errors should be fixed as well.

@zhangguanheng66
Copy link
Contributor

You can use flake8 and run the lint error tests locally.

with self.assertRaises(NotImplementedError):
utils.extract_archive(archive_path)

# remove file

This comment was marked as resolved.

Copy link
Contributor

@zhangguanheng66 zhangguanheng66 left a comment

Choose a reason for hiding this comment

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

In python2, exist_ok arguments doesn't exist in makedirs() func. If you really need it, you can check the python version (example here)

@zhangguanheng66
Copy link
Contributor

@bentrevett Thanks for the contributions. Nice jobs. I just update the PR with the master branch. Will merge it after CI tests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants