Skip to content

Conversation

@JavierMtzRdz
Copy link

@JavierMtzRdz JavierMtzRdz commented Nov 4, 2025

Checklist

Please:

  • Make sure this PR is against "dev", not "main" (unless this is a release
    PR).
  • Request a review from one of the current epidatr main reviewers:
    brookslogan, dshemetov, nmdefries, dsweber2.
  • Makes sure to bump the version number in DESCRIPTION. Always increment
    the patch version number (the third number), unless you are making a
    release PR from dev to main, in which case increment the minor version
    number (the second number).
  • Describe changes made in NEWS.md, making sure breaking changes
    (backwards-incompatible changes to the documented interface) are noted.
    Collect the changes under the next release number (e.g. if you are on
    1.7.2, then write your changes under the 1.8 heading).

Change explanations for reviewer

  • Change: Provide a clearer explanation as in the message linked to the function.
  • Change: Provides a clearer explanation.
  • Change: Adds a .epidatr_shared_params list of parameters that includes shared parameters and inherits shared parameters in the specific functions' documentation.
  • Change: Adds a Data Versioning section to clarify its use.

Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch

JavierMtzRdz and others added 8 commits November 2, 2025 15:01
It clarifies that the API key needs to be manually updated and closes issue 295.
This closes issue 287 since the location does not need to be specified.
…Versioning section

To reduce repetition, epidatr_shared_params contains shared parameters that are inherited by the functions’ documentation. Data Versioning is also inherited where necessary.
This change prevents issues with referencing .epidatr_shared_params when building the documentation site.
#' @param auth string. Your restricted access key (not the same as API key).
#' @param locations character. Vector of locations to fetch.
#' @param states character. Two letter state abbreviations.
#' @param regions character. Vector of regions to fetch.
Copy link
Contributor

@dshemetov dshemetov Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Thanks for taking these issues on!

question: Does R intelligently notice when a "shared parameter" isn't present in a function and leave it off? If yes, then consolidating all params up here sounds great. If not, then will this list arguments that don't apply to various endpoints? For instance, pub_covidcast doesn't have regions or states or locations, do those still show up? It might not be a big deal if there are extra arguments, just wondering what our options are for this sort of thing.

Copy link
Author

@JavierMtzRdz JavierMtzRdz Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

answer: Yes, "@inheritParams only inherits docs for arguments that the function actually uses and that aren’t already documented". Also, there are a few cases where epiweeks or regions have a different description. In those cases, roxygen2 overrides the inherited parameter description with the provided description for that function.

Also, I reviewed the documentation to ensure it was working as intended, and it is. pub_fluview_clinical as an example below.
Screenshot 2025-11-05 at 12 42 25 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: for fluview_clinical regions, I don't see the link to the full region list.

suggestion: make fluview and fluview_clinical regions description more specific. E.g., I think something like

#' @param regions character. Vector of location IDs to fetch.  Can be
#'   "nat" for national, "hhs1"--"hhs10" for HHS Regions, "cen1"--"cen9" for census divisions,
#'   lowercase two-letter state or territory abbreviations for most states and territories,
#'   "jfk" for New York City, or "ny_minus_jfk" for upstate New York.
#'   Full list of locations is available <here>.

w/ an actual link.

Copy link
Author

@JavierMtzRdz JavierMtzRdz Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed: I incorporated the detailed description you proposed. Regarding the link, I wanted to create a section within the Epidata API Documentation website, but while that is being arranged, I linked it to the regions' labels in the repository.

#' @param locations character. Vector of locations to fetch.
#' @param states character. Two letter state abbreviations.
#' @param regions character. Vector of regions to fetch.
#' @param epiweeks [`timeset`]. Epiweeks to fetch. Supports
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: fix content that got cut off here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@JavierMtzRdz
Copy link
Author

JavierMtzRdz commented Nov 7, 2025

I added changes to address issues #208 and #288.

Change explanations for reviewer

  • Change: I added conditional examples with @examplesIf so they only run when there is internet access and the corresponding API key is available. I did not use \donttest as it is checked by CRAN occasionally. The examples on the documentation website appear when the API key is available. Examples with lengthy outputs are enclosed within \dontrun safeguards.
  • Note: All examples worked. The only relevant comment is that the pub_flusurv example returns data with a warning.

Warning message:
Not all return columns are specified as expected epidata fields
ℹ Unspecified fields season, rate_age_5, rate_age_6, rate_age_7, rate_age_18t29,
rate_age_30t39, rate_age_40t49, rate_age_5t11, rate_age_12t17, rate_age_lt18,
rate_age_gte18, rate_age_1t4, rate_age_gte75, rate_age_0tlt1, rate_race_white,
rate_race_black, rate_race_hisp, rate_race_asian, …, rate_flu_a, and rate_flu_b may
need to be manually converted to more appropriate classes

Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch

@dshemetov
Copy link
Contributor

dshemetov commented Nov 8, 2025

Request: the Data Versioning section is good, but can we also add two more examples to the Getting Started vignette to address my suggestion in #199? Specifically of using the issues field and the lag field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants