-
Notifications
You must be signed in to change notification settings - Fork 5
Make endpoints fetch all dates by default #234
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
Conversation
rename wildcard conversion type to "week"
`pub_covid_hosp_facility` says and implies it takes weeks for the arg `collection_weeks`, but it actually only takes days (corresponding to weeks) and will error if given weeks. Given the misleading name and behavior, and differing behavior compared to other endpoints with weekly data, allow users to supply weeks -- these will be converted to dates.
refactor reformat_epirange; add date_to_epiweek helper fn better collection_weeks message
ea16f73
to
b5e58c7
Compare
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.
High level skim, looks good to me. Might be good to add a changelog line now so you don't have to later.
be careful with this; two months from now, if just 2 county-level daily signals that started January 2020 are requested simultaneously, they could exceed the row limit: |
Good point. Maybe add a warning about county=* queries? |
Thank for the information, george. Good to know we have a truncation warning already set up. I believe only Ideally, these endpoints would support "*" for geos, but that will involve a lot more work. I'll keep the row limit in mind if/when we get to that! |
…i-twitter Combine timetype wiki twitter
Partially addresses #212.
Fetch all dates (time values) by default, using "*". Also support user-provided "*" with addition of helper function to convert "*" into a long
epirange
.Given confusing naming of date params for
pub_covid_hosp_facility
, wherecollection_weeks
actually requires YYYYMMDD format, let users provide weeks (with a warning).Note:
pub_wiki
andpvt_twitter
aren't updated here. They both take either days or epiweeks (mutually exclusive), so I'm proposing an interface change first in #236.