Summary
While unifying the versioning-parameter handling across endpoints, I noticed that four endpoints guard issues/lag exclusivity with !missing(issues) && !missing(lag), which rejects explicit NULL arguments. In R, missing() is about the call site, not the value — so pub_flusurv(locations = "ca", issues = NULL, lag = 1) stops with a hard error before any network call, although issues = NULL means "not requested". This bites programmatic callers that build argument lists (e.g. do.call/pmap pipelines with NULL defaults for optional filters). Five other endpoints already use the correct NULL-aware pattern; I think the four should be switched to it for consistency with the documented contract.
Evidence (dev tip 87b10e1; same checks on main)
The missing()-based check appears in exactly four endpoints:
R/endpoints.R:2247-2249 (pub_ecdc_ili)
R/endpoints.R:2316-2318 (pub_flusurv)
R/endpoints.R:2409-2411 (pub_fluview_clinical)
R/endpoints.R:2703-2705 (pub_kcdc_ili)
if (!missing(issues) && !missing(lag)) {
stop("`issues` and `lag` are mutually exclusive")
}
The NULL-aware pattern, used correctly by:
pub_fluview (R/endpoints.R:2523-2525)
pub_nidss_flu (R/endpoints.R:2868-2870)
pub_covidcast (R/endpoints.R:1313-1317, issues/lag/as_of triple)
Maintainer-runnable repro
pub_flusurv(locations = "ca", issues = NULL, lag = 1)
# Error: `issues` and `lag` are mutually exclusive
# (raised before any network call; the identical call with no `issues` argument succeeds)
pub_nidss_flu(regions = "ca", issues = NULL, lag = 1) # works fine — inconsistent
Expected vs. actual
- Expected:
issues = NULL means "not requested" and is treated the same as omitting the argument, everywhere.
- Actual: four endpoints reject the explicit-NULL form with a spurious hard error, while the rest of the package accepts it.
Suggested fix
Replace the missing()-based check with the NULL-aware one in all four:
if (!is.null(issues) && !is.null(lag)) {
stop("`issues` and `lag` are mutually exclusive")
}
(validate_timeset_input(..., required = FALSE) already normalizes non-NULL values, so the is.null() form is safe.)
I checked for prior reports (open/closed issues/PRs about issues/lag handling) and found none covering this.
Summary
While unifying the versioning-parameter handling across endpoints, I noticed that four endpoints guard
issues/lagexclusivity with!missing(issues) && !missing(lag), which rejects explicitNULLarguments. In R,missing()is about the call site, not the value — sopub_flusurv(locations = "ca", issues = NULL, lag = 1)stops with a hard error before any network call, althoughissues = NULLmeans "not requested". This bites programmatic callers that build argument lists (e.g.do.call/pmappipelines withNULLdefaults for optional filters). Five other endpoints already use the correct NULL-aware pattern; I think the four should be switched to it for consistency with the documented contract.Evidence (dev tip 87b10e1; same checks on
main)The
missing()-based check appears in exactly four endpoints:R/endpoints.R:2247-2249(pub_ecdc_ili)R/endpoints.R:2316-2318(pub_flusurv)R/endpoints.R:2409-2411(pub_fluview_clinical)R/endpoints.R:2703-2705(pub_kcdc_ili)The NULL-aware pattern, used correctly by:
pub_fluview(R/endpoints.R:2523-2525)pub_nidss_flu(R/endpoints.R:2868-2870)pub_covidcast(R/endpoints.R:1313-1317,issues/lag/as_oftriple)Maintainer-runnable repro
Expected vs. actual
issues = NULLmeans "not requested" and is treated the same as omitting the argument, everywhere.Suggested fix
Replace the
missing()-based check with the NULL-aware one in all four:(
validate_timeset_input(..., required = FALSE)already normalizes non-NULL values, so theis.null()form is safe.)I checked for prior reports (open/closed issues/PRs about
issues/laghandling) and found none covering this.