-
Notifications
You must be signed in to change notification settings - Fork 0
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 csv endpoint wrapper #38
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a quick scan through and looks good. Was a bit thrown when you hadn't just added a specific "get-csv" endpoint to api_url, which would have been my first thought, but think this way makes sense.
Main thought is handling the additional potential for redundant parameters in api_url()
. I flag a warning if the time_periods, indicators, etc parameters are set when someone's running an endpoint that doesn't use them, e.g. "get-meta". At the very least I think that flag should trigger for this scenario as well. Plus response_format = CSV doesn't apply to any other endpoint, so should probably be a warning if someone sets it to CSV for any of those requests.
I'm a little wary that if this is the only endpoint that's ever going to be able to deliver a csv, then response_format is a bit redundant and just "get-csv" as an endpoint might make more sense and make the code simpler in terms of having one less thing that doesn't do anything across all the other endpoints.
I guess on the above "expecting we can build on this more if the CSV response type gets interest and requests for more features" is carrying the weight here, so if we think that's likely, I guess just adding in some warnings would be helpful.
Interesting to see where your brain instinctively jumped to. I was in two minds when I approached it and this was enough to make me go back and review it a bit more in the API docs. While in my head this might in the long run just be an option on the Have also made sure the warnings appear for unecessary arguments and added a quick test for it too. |
Brief overview of changes
Add a wrapper function for getting a CSV version of an API data set from EES.
Why are these changes being made?
There's a GET endpoint for datasets where you can specify CSV as the response type. There's no querying or even dataset version options on this endpoint yet so it's literally just a download of the latest version.
Detailed description of changes
Updated
api_url()
to have a new response_type argument where you can switch between JSON or CSV, expecting we can build on this more if the CSV response type gets interest and requests for more features.I figured
api_url()
made the most sense for these changes, though happy to be told this jars with a different vision you have @rmbielby - aware you've put a lot of thought into the modularising of the code in the package.Additional information for reviewers
Couple of small bits of tidy up.
Did this as there were messages appearing in
devtools::check()
about this related to the test data we have in the package.Debated adding an on load warning for users like this in a zzz.R file:
But decided against as overkill, as it probably won't affect most users.
httr uses readr behind the scenes, not including it was causing errors when running tests / examples, but including as a full import was giving a warning as we never call readr directly. This seemed to work as a solution that appeased the R package gods.
toggle_message()
functionNow, I know we have this in dfeR, though I wanted to avoid having all of dfeR and it's dependencies and dependencies on this.
Felt like a small enough function to copy over and stick in a utils.R file for us to use ourselves to control verbosity. Expect we'd take a similar approach with other packages too.
We were exceeding on a couple of functions. I think we're mostly safe with what we're doing for now and are modularising a lot already so a higher limit of 25 seemed more suitable and hushed lintr for the time being.
usethis::use_tidy_description()
It moved some stuff about in the DESCRIPTION file.
Issue ticket number/s and link
Resolves #36.