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

Combine timetype wiki twitter #236

Merged

Conversation

nmdefries
Copy link
Contributor

@nmdefries nmdefries commented Jan 9, 2024

pvt_twitter and pub_wiki can take dates in either day or week format. These are provided as separate, mutually exclusive (will fail) params dates and epiweeks. This format disagrees with the interface for pub_covidcast, which also supports days and weeks. Change the twitter and wiki interfaces to match, with new params time_type and time_values.

This also makes supporting the date range default to wildcard "*" (all dates) easier -- if both dates and epiweeks params were set to "*" by default, the user would have to specify which date type they wanted to use anyway, a la time_type, since we wouldn't be able to automatically determine precedence.

Copy link

pullflow-com bot commented Jan 10, 2024

nmdefries (GitHub user) ▸ 🔘 Marked ready for review

Copy link

pullflow-com bot commented Jan 10, 2024

nmdefries (GitHub user) ▸ 🔍 Review requested from dshemetov, dsweber2 and brookslogan

2 similar comments
Copy link

pullflow-com bot commented Jan 10, 2024

nmdefries (GitHub user) ▸ 🔍 Review requested from dshemetov, dsweber2 and brookslogan

Copy link

pullflow-com bot commented Jan 10, 2024

nmdefries (GitHub user) ▸ 🔍 Review requested from dshemetov, dsweber2 and brookslogan

Copy link
Contributor

@dsweber2 dsweber2 left a 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 uses dates, matching the API
  • pub_delphi does actually use epiweeks 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, and pub_nidss_flu
  • I'm not actually sure about pvt_norostat. The docs have ellipses covering the date.
  • pub_nowcast, as epiweeks matches their api docs
  • pub_paho_dengue has ellipses covering the date
  • pvt_quidel, pvt_sensors, use epiweeks in both

fetch_args = fetch_args_list()) {
rlang::check_dots_empty()

time_type <- match.arg(time_type)
Copy link
Contributor

@dsweber2 dsweber2 Jan 10, 2024

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)

Copy link
Contributor

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.

Copy link

pullflow-com bot commented Jan 10, 2024

dsweber2 ▸ ⎘ R/endpoints.R: 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)

@nmdefries nmdefries merged commit 2657bed into ndefries/endpt-defaults Jan 18, 2024
1 check passed
@nmdefries nmdefries deleted the ndefries/combine-timetype-wiki-twitter branch January 18, 2024 22:44
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.

None yet

4 participants