Error trapping for excessive durations in coops_search() coops.R #214
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added test for requests for longer-duration chunks of data than the NOAA API will provide. Added notice to comment block at top.
fixed extra close line 149parens
datums product does not in fact _Require_ begin_date & end_date, and coops_search() does not test !is.null() on those parameters. Note also that station_name is listed as "numeric", but it clearly can be character. For product="currents", it _must_ be character, as the station_name values for all stations with currents data start with an alphabetic character, not a numeral. See the example on the man page: '''coops_search(station_name = "ps0401", begin_date = 20151221, end_date = 20151222, product = "currents")'''
added 2 tests of failing maximum durations lines 45-64
|
thanks for this @tphilippi having a look |
sckott
suggested changes
Apr 25, 2017
There was a problem hiding this comment.
nice work!
Make sure to put spaces between symbols, like x = y not x=y
The first example coops_search(station_name = 8723970, begin_date = 19820301, end_date = 20141001, datum = "stnd", product = "monthly_mean") fails now with your code. That example doesn't fail with the previous version of this fxn, so curious if the rule you put internally should be changed or not?
R/coops.R
Outdated
| #' Products water_level through predictions allow requests for up to 31 days of data. | ||
| #' Products hourly_heignt and high_low allow requests for up to 1 year (366 days) of data. | ||
| #' Products daily_mean and monthly_mean allow requests for up to 10 years of data. | ||
| # ' |
R/coops.R
Outdated
| #' \item datums - datums data for the stations | ||
| #' \item currents - Currents data for currents stations | ||
| #' } | ||
| #' | ||
| #' Maximum Durations in a Single Call: | ||
| #' Products water_level through predictions allow requests for up to 31 days of data. |
There was a problem hiding this comment.
for these lines 49-51, don't go over 80 character width, so jus wrap to next line if needed.
and put them in a list like
#' \itemize{
#' \item xxx
#' \item xxx
#' \item xxx
#' }
R/coops.R
Outdated
| "air_pressure", "air_gap", "conductivity", "visibility", "humidity", | ||
| "salinity", "one_minute_water_level", "predictions", "currents") | ||
|
|
||
| group2_products <- c( # hourly to sub-daily products with 1 year max |
There was a problem hiding this comment.
less than 80 character width
R/coops.R
Outdated
|
|
||
|
|
||
| # check for duration longer than NOAA will return | ||
| group1_products <- c( # sub-hourly products with 31 day max |
There was a problem hiding this comment.
less than 80 character width
R/coops.R
Outdated
|
|
||
| group2_products <- c( # hourly to sub-daily products with 1 year max | ||
| "hourly_height", "high_low") | ||
| group3_products <- c( # daily or longer products with 10 year max |
There was a problem hiding this comment.
less than 80 character width
R/coops.R
Outdated
| ed <- as.Date(as.character(end_date),format="%Y%m%d") | ||
| req_dur <- ed - bd | ||
| if (req_dur > maxdur) { | ||
| stop(paste("The maximum duration NOAA API supports a single call for\n", product, " is ", maxdur, " days\n", |
There was a problem hiding this comment.
less than 80 character width
R/coops.R
Outdated
| maxdur <- 3653 | ||
| } else maxdur <- 365000 | ||
|
|
||
| if (!is.null(begin_date)&!is.null(end_date)) { |
sckott
approved these changes
Apr 27, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Added trapping for begin_date and end_date combinations requesting longer durations than the NOAA API will accept for that product. Instead of returning generic (and incorrect or at least misleading) "No data found", coops_search() now fails with an informative error message.
Re-ordered product item order into 31-day max, 1 year max, 10 year max, and no dates (datums) and added block of text noting the maximum durations in single calls. I DID NOT TEST WHETHER THIS RENDS CORRECTLY! Please check before accepting the pull request.
Added 2 tests at bottom of tests/testthat/test-coops.R. My full test suite includes a pair of calls for each product, one with short duration that should return data and one with excessive duration that should throw error instead of "No data found". I don't know your policy on exhaustiveness of tests, especially given that they all hit an external API.
Related Issue
(Mostly) fix issue #213
Example
2 tests added to tests-coops.R