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

Add function for fetching in progress responses, closing #220 #261

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jamesmartherus
Copy link

Adds a function for downloading in progress responses. Closes #220

@jamesmartherus jamesmartherus marked this pull request as ready for review June 8, 2022 02:12
Copy link
Collaborator

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

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

Thank you so much for this contribution @jamesmartherus! 🙌

One thing I notice about this PR is that it copies a fair amount of code from fetch_survey(). Instead of copy/pasting so much, can we create some helper functions that can be used for both functions?

The other thing we'll need here are some tests. We use vcr for testing, as outlined in rOpenSci's book on HTTP testing.

R/fetch_inprogress.R Outdated Show resolved Hide resolved
R/fetch_inprogress.R Outdated Show resolved Hide resolved
R/fetch_inprogress.R Outdated Show resolved Hide resolved
R/fetch_inprogress.R Outdated Show resolved Hide resolved
@jamesmartherus
Copy link
Author

Thanks @juliasilge!

I've addressed the deprecation and naming comments and added some tests. Great point about using helpers instead of copying so much code. One potential helper function could check if the survey is already in tempdir?

The reason I ended up copying most of the fetch_survey() code is that it already does a really good job using helper functions for checking parameters, generating the URL, building the request, etc. Did you have some other sorts of functions in mind?

@jmobrien
Copy link
Collaborator

Hey, just seeing this--this is great to have, thanks! And nice catch on avoiding the saved files overwriting any from fetch_survey()

FYI, I just put in a PR adding features to fetch_survey() that also also tried to make the function a bit more modular (and restructured arg checking). This includes a set of internal helper functions for the export-responses endpoint. I think that could be useful as a drop-in for both fetch_survey() and fetch_inprogress that could help streamline things w/o too much effort.

@juliasilge since these two functions are basically the same call, we could in theory consolidate just about everything into a shared internal function except the arg checks (if we're dropping the start/end args), payload/body build (same reason), and the file paths. But I don't know how far you might want to go on DRY.

Merge branch 'master' into jamesmartherus-master

# Conflicts:
#	R/utils.R
#	man/create_raw_payload.Rd
@juliasilge
Copy link
Collaborator

@jamesmartherus Thank you again so much for this new feature! Are you interested in making the changes based on #263? I do think @jmobrien's idea of making one internal function that is used by both of these user-facing functions is the right way to go, for long-term maintainability and avoiding bugs down the road.

@jmobrien
Copy link
Collaborator

There's a function called export_responses_request() that should work for that now.

@juliasilge
Copy link
Collaborator

I just moved this repo's default branch from master to main; you can use usethis::git_default_branch_rediscover() to update your local setup.

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.

Feature: Option to download in progress responses
3 participants