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

Attempt to add extract_dir specification #72

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cansavvy
Copy link

@cansavvy cansavvy commented Oct 14, 2021

Problem

This is to potentially close #71 and have downloaded files extract to the path specified by the user as the docs have me believe is what was intended to happen.

Approach

I added path as a parameter to both extract functions and set extract_dir = path in the shutil.unpack_archive.

Then I ran python -m unittest discover tests -b as the README said to do. I got three errors, but I also got those same three errors on the master branch, so I don't think they are related to the changes I made here.

That being said, even though the changes didn't cause errors, I don't know if this will work correctly and I don't know how to test if it will fix #71.

Also don't even know if I am correct that #71 is a problem. ¯_(ツ)_/¯

Notes reviewers

You should look into my changes more carefully and make sure that they work. I am a Python newb and don't know what I'm doing. Also not very familiar with this code base. But I wanted to take a whack at it because I'd like this to work how the docs described it.

Copy link
Contributor

@kurtwheeler kurtwheeler left a comment

Choose a reason for hiding this comment

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

Hi Candace! Sorry, didn't mean to ignore this for so long. I wasn't watching all activity on this repo but I've changed that so I get notified even for things not assigned to me!

Anyway, this looks like a great change. The only tweak I'd suggest is to make path an optional parameter and then only supply it if it's not None. According to the docs for shutil.unpack_archive,

extract_dir is the name of the target directory where the archive is unpacked. If not provided, the current working directory is used.

It looks like in the screenshot you provided in #71 that it may have been extracted into your Python-examples directory instead of data. I agree that the user should be able to provide a path, but I like that the default is the working directory.

Also, I got the tests passing again, so if you pull these should pass as well.

@kurtwheeler
Copy link
Contributor

Wait! I've looked at this a little more since my dataset actually downloaded. I guess it doesn't really make sense to say that you want to extract something but not give a path where it should go, especially when we're already requiring a path for the dataset zip to be downloaded to.

So I think these changes are actually solid as they are! However, I don't think anything in this PR is actually using them. The user can't pass them through the download_dataset and download_compendium functions because those aren't accepting the parameter. However, both those functions already have a boolean extract parameter. I think we should change that extract parameter to be an optional string extract_destination parameter. If that parameter is supplied we use it as the path, otherwise we don't extract.

@cansavvy
Copy link
Author

cansavvy commented Oct 22, 2021

I think I understand what this would look like, and am happy to give it a try, but if you want to make this change more quickly, it would definitely be more efficient for you or someone else who's more familiar with repo to do it. Up to you.

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.

extract argument didn't work as expected
2 participants