-
Notifications
You must be signed in to change notification settings - Fork 5
Combine timetype wiki twitter #236
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
Combine timetype wiki twitter #236
Conversation
This makes the pub_wiki interfaces better match that of the other endpoints. Also build in time_values default of "*".
nmdefries (GitHub user) ▸ 🔘 Marked ready for review |
nmdefries (GitHub user) ▸ 🔍 Review requested from dshemetov, dsweber2 and brookslogan |
2 similar comments
nmdefries (GitHub user) ▸ 🔍 Review requested from dshemetov, dsweber2 and brookslogan |
nmdefries (GitHub user) ▸ 🔍 Review requested from dshemetov, dsweber2 and brookslogan |
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.
The tests cover what I would think of. Only 1 minor note.
I'm a bit surprised that these weren't following the convention of the other endpoints. Looking for other exceptions, I didn't find any, thankfully. Good catch!
- CDC matches the api in using
epiweeks
pub_covid_hosp_state_timeseries
usesdates
, matching the APIpub_delphi
does actually useepiweek
s only- same for
pub_dengue_nowcast
,pvt_dengue_sensors
,pub_ecdc_ili
,pub_flusurv
,pub_fluview_clinical
,pub_fluview
,pub_gft
,pub_kcdc_ili
,pub_nidss_dengue
, andpub_nidss_flu
- I'm not actually sure about
pvt_norostat
. The docs have ellipses covering the date. pub_nowcast
, asepiweeks
matches their api docspub_paho_dengue
has ellipses covering the datepvt_quidel
,pvt_sensors
, useepiweeks
in both
fetch_args = fetch_args_list()) { | ||
rlang::check_dots_empty() | ||
|
||
time_type <- match.arg(time_type) |
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.
I'm not entirely sure this is necessary, or else pub_covidcast
is missing something important, since it doesn't do this. I would maybe make a function for this bit of logic if it is necessary, since it (should?) be included 3 times.
(I mean this and if
statements below)
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.
Here it's necessary because the default value is c("day", "week")
, but you can only have one time_type
. match.arg()
takes the first if the default is not overridden. It's a common convention in R that if the argument can be one of several string values, you make the default list all of them, and match.arg()
takes care of picking the first (if none is provided) or ensuring the provided value is valid otherwise.
pub_covidcast()
doesn't set a default for this argument, so it's not strictly necessary. However, pub_covidcast()
could do match.arg(time_type, c("day", "week"))
to enforce that the type must be one or the other but not both.
dsweber2 ▸ ⎘ R/endpoints.R: I'm not entirely sure this is necessary, or else (I mean this and |
pvt_twitter
andpub_wiki
can take dates in either day or week format. These are provided as separate, mutually exclusive (will fail) paramsdates
andepiweeks
. This format disagrees with the interface forpub_covidcast
, which also supports days and weeks. Change the twitter and wiki interfaces to match, with new paramstime_type
andtime_values
.This also makes supporting the date range default to wildcard "*" (all dates) easier -- if both
dates
andepiweeks
params were set to "*" by default, the user would have to specify which date type they wanted to use anyway, a latime_type
, since we wouldn't be able to automatically determine precedence.