Skip to content

Extreme requests #132

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

Merged
merged 8 commits into from
Jul 8, 2023
Merged

Extreme requests #132

merged 8 commits into from
Jul 8, 2023

Conversation

dsweber2
Copy link
Contributor

@dsweber2 dsweber2 commented Jul 5, 2023

Adds two parameters to several functions:

  • timeout_seconds gives the maximum time before a connection will time out. Default is 30 seconds, which matches the value returned in the call below (can't find documentation for httr's default, since it clearly isn't using libcurl's). Originally motivated by
    chng_outpatient <- epidatr::covidcast(
      data_source = "chng", signals = "smoothed_adj_outpatient_covid",
      geo_type = "state",
      time_type = "day",
      time_values = "*",
      geo_values = "*",
      issues = epirange("2020-11-16","2021-03-16")
    ) %>% fetch()
    
    which returns a timeout message, trying 3 times. A working version in this branch is
    chng_outpatient <- epidatr::covidcast(
      data_source = "chng", signals = "smoothed_adj_outpatient_covid",
      geo_type = "state",
      time_type = "day",
      time_values = "*",
      geo_values = "*",
      issues = epirange("2020-11-16","2021-03-16")
    ) %>% fetch(timeout_seconds = 2.5 * 60)
    
    I'm not sure there's a nice way to add a unit test for something like this however. Perhaps httptest has something.
  • return_empty, which handles the case of no returned results whatsoever. Defaults to FALSE, which is the current value. Even if its true it throws a warning. We may want to consider making TRUE the default. I debated adding headers to empty columns, but it ended up being more awkward to use on the other end.
    Should be relatively easy to add tests, but I haven't yet. It does work in the pipeline that motivated it (breaking the above into months, some of which chng has no data).

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (dev@f121d72). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 4ad7379 differs from pull request most recent head cfe5521. Consider uploading reports for the commit cfe5521 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@          Coverage Diff           @@
##             dev     #132   +/-   ##
======================================
  Coverage       ?   95.37%           
======================================
  Files          ?        8           
  Lines          ?     1168           
  Branches       ?        0           
======================================
  Hits           ?     1114           
  Misses         ?       54           
  Partials       ?        0           

Copy link
Contributor

@dshemetov dshemetov left a comment

Choose a reason for hiding this comment

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

looks good!

  • tests for timeout_seconds not necessary, since the codepath is still the same
  • a single test for return_empty might be good, but up to you, purely a code cov thing

@dshemetov
Copy link
Contributor

#128

@dsweber2 dsweber2 merged commit 032b6d7 into cmu-delphi:dev Jul 8, 2023
@dsweber2 dsweber2 mentioned this pull request Jul 12, 2023
@dsweber2 dsweber2 added this to the Z: epidatr & epidatpy milestone Oct 23, 2023
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.

3 participants