Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd ability to download zip of multiple files #47
Conversation
This comment has been minimized.
This comment has been minimized.
This is cool. For now, do you mind submitting a PR that fixes the regression, but leaves out the zip function until we get some consensus around #48? |
fix multi-download for datasets with folders and change logic
This comment has been minimized.
This comment has been minimized.
I've reverted the zip feature as requested and will chime in on #48 In writing tests, I noticed that the current function, which was written before zip downloads included folder hierarchies, doesn't work for datasets with folders. Given that @pdurbin also suggests in #46 that there are downsides to requesting the zip and unpacking it, I've re-written the logic to download the files sequentially instead, not using the zip functionality of the API at all. That passes all tests and seems to perform faster even with small datasets. If this looks good, I can clean up a bit more, but I think the tests are pretty good as they are. |
This comment has been minimized.
This comment has been minimized.
That's what I ultimately recommended at jupyter/repo2docker#739 (comment) |
adam3smith commentedJan 10, 2020
•
edited
Please ensure the following before submitting a PR:
/R
not/man
and rundevtools::document()
to update documentation/tests
for any new functionality or bug fixR CMD check
runs without error before submitting the PRThis PR does three things:
Adds an additional parameter(see below)unzip
toget_file()
to give users the option to download individual files (the current behavior) or a single .zip file (my preferred behavior).I'll add more tests if you're OK with the general approach here, but wanted to check first. I'd also need access to the dataverse on demo to properly test (all tests using the demo DV currently fail as I assume you know)