Closed cjrace closed 1 month ago
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 /query
endpoints for response type, currently in the API itself it has been implemented as a separate /csv
endpoint so for now I've switched over to that, treating it as a get-csv
endpoint in the code, and we can worry about response_type
at a later point once we know the future shape of the API better.
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.