search

ropensci-archive / bomrang

:warning: ARCHIVED :warning: Australian government Bureau of Meteorology (BOM) data client for R
Other
109 stars 26 forks source link

First pass at empty BOM station list fix #121

Closed jimjam-slam closed 4 years ago

jimjam-slam commented 4 years ago

Description

This approach makes minimal changes to .get_ncc based on @paulr-bv's proposal, but it detours through base::readLines in order to identify the header and footer rows before passing to readr::read_table.

If the table doesn't exist because there are no rows, a warning is thrown. Because of the current implementations of .get_ncc and get_historical_weather, this means a warning is thrown even if you're not requesting the type of data for which the station list is missing. This might be confusing for users!

An alternate approach would be to change .get_ncc to accept a data type argument and to only request the relevant station list. This kind of change would be pretty small, and it seems like it's only used in the context of get_historical_weather, so it seems like a good way to go. Is hoardr used with station data? If so, would such a change mess with that?

Related Issue

Fixes #119; fixes #120.

jimjam-slam commented 4 years ago

Note that some of the tests for get_historical_weather use rainfall, so automated checks aren't passing:

== testthat results  ===========================================================
[ OK: 244 | SKIPPED: 0 | WARNINGS: 12 | FAILED: 4 ]
1. Failure: Query of 'castlem' and friends (@test-get_current_weather.R#39) 
2. Error: Error handling (@test-get_historical.R#13) 
3. Error: Query stationid = '023000',
          type = 'rain' returns bomrang_tbl w/ correct station and some data (@test-get_historical.R#23) 
4. Error: Query latlon = c(-34.9285, 138.6007),
          type = 'rain' returns bomrang_tbl w/ correct station and some data (@test-get_historical.R#46)
adamhsparks commented 4 years ago

We could simply request the desired data. That change wouldn't affect anything but this function. But this should be sufficient I think. I'll work on the tests.

Right now hoardr is only used with imagery, so it wouldn't interfere with this. These files would change over time if you re-query historical data from a still operating station, so I don't think it's useful to use here.

jimjam-slam commented 4 years ago

Makes sense! If I get some time down the road, I'll look at another PR to rework the function to just request the desired data 😊