Skip to content
Please note that GitHub no longer supports your web browser.

We recommend upgrading to the latest Google Chrome or Firefox.

Learn more

/ dataverse-client-r

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

Add ability to download zip of multiple files #47

Open
wants to merge 2 commits into
base: master
from
Open

Add ability to download zip of multiple files #47

wants to merge 2 commits into from
+58 −27

Conversation

@adam3smith
Copy link
Contributor

adam3smith commented Jan 10, 2020 •
edited

Please ensure the following before submitting a PR:

  • if suggesting code changes or improvements, open an issue first
  • for all but trivial changes (e.g., typo fixes), add your name to DESCRIPTION
  • for all but trivial changes (e.g., typo fixes), documentation your change in NEWS.md with a parenthetical reference to the issue number being addressed
  • if changing documentation, edit files in /R not /man and run devtools::document() to update documentation
  • add code or new test files to /tests for any new functionality or bug fix
  • make sure R CMD check runs without error before submitting the PR

This PR does three things:

  1. Fixes a regression from 5ec375b that broke the ability to specify a vector of fileids to download
  2. Documents and test said functionality
  3. Adds an additional parameter unzip to get_file() to give users the option to download individual files (the current behavior) or a single .zip file (my preferred behavior). (see below)

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)
@adam3smith
Add ability to download zip of multiple files
Loading status checks…
d38ed88
@wibeasley wibeasley mentioned this pull request Jan 11, 2020
Open
@wibeasley

This comment has been minimized.

Sign in to view
Copy link
Contributor

wibeasley commented Jan 11, 2020 •
edited

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?
@adam3smith
revert zip of multiples
Loading status checks…
21bb003 
fix multi-download for datasets with folders and change logic
@adam3smith

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

adam3smith commented Jan 11, 2020

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.
@pdurbin

This comment has been minimized.

Sign in to view
Copy link
Member

pdurbin commented Jan 11, 2020

I've re-written the logic to download the files sequentially instead, not using the zip functionality of the API at all.

That's what I ultimately recommended at jupyter/repo2docker#739 (comment) 😄
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@adam3smith @wibeasley @pdurbin
You can’t perform that action at this time.