ropensci-archive / bomrang

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

Best way for get-current_weather to handle station IDs? #103

Closed jimjam-slam closed 1 year ago

jimjam-slam commented 5 years ago

get_current_weather() currently accepts either a station_name (that is precisely and then fuzzy matched) or a latlon vector:

get_current_weather(station_name, strict = FALSE, latlon = NULL, emit_latlon_msg = TRUE)

I was thinking of submitting a PR to allow this function to accept a stationid, like get_historical() does, since the two functions ultimately use the same JSONurl_site_list to look stations up. I'm not sure (if you were to take this PR, that is), whether you'd prefer an extra argument be added to the function's formals , or whether you'd just prefer for station_name to be checked against both columns JSONurl_site_list.

get_historical(), in turn, doesn't match against names, only IDs. So maybe there's an API conversation there. What do you think?

deanmarchiori commented 5 years ago

Agree standardizing the API where possible sounds like a good idea.

It seems like using station_id may be more precise? I wonder whether there was a reason why station_name was used in get_current_weather()? I can see in the function details there are some disclaimers around its use:

station names are not consistently named within the Bureau, so the response may contain a different full_name to the one matched, even if strict = TRUE.

adamhsparks commented 5 years ago

Disclaimer I didn’t write either of these functions, but part of it is having several developers working on the same package I would guess.

I do think it would be good to standardise the parameters across functions as much as we can, but we need to remain mindful of the BOM terminology too. In some cases that differs from product to product, and where it does I’d prefer to stick to their nomenclature. So no issues here if we can put something together to clean it up. I think that makes things easier for end users all the way around.

@PaulMelloy has been working on a few other enhancements for get_historical() as well to make it easier to batch download data and get_precis_forecast() to allow importing local data where it may be necessary to download the data outside of R. It would be good to get all of these implemented in a new release.

adamhsparks commented 4 years ago

I've just submitted a new minor release to CRAN that fixes a few bugs and more importantly keeps bomrang on CRAN due to failing some checks.

There are some fairly major changes waiting in the devel branch right now if we wanted to roll this into the next release with those.

adamhsparks commented 4 years ago

@rensa, are you thinking to submit a PR for this still or should I go ahead and start working on it for the next release?

jimjam-slam commented 4 years ago

I'm sorry, Adam—I'd love to do the PR, but the way I'm going at the moment, it might be a few weeks before I get the bandwidth 😔 If you're keen to get the next release out the door, I'm not sure I can contribute in time!

adamhsparks commented 4 years ago

No hurry at all. See the issue I just opened for v1.0.0. This PR is included in that and I'm in no hurry after releasing 0.7.0 on CRAN today.

-- Dr Adam H. Sparks Associate Professor of Field Crops Pathology  |  Centre for Crop Health Institute for Life Sciences and the Environment (ILSE)  |  Research and Innovation Division University of Southern Queensland | Toowoomba, Queensland | 4350 | Australia

Phone: +61746311948 Mobile: +61415489422 Mobile: +61415489422 On 21 Jan 2020, 10:26 +1000, James Goldie notifications@github.com, wrote:

I'm sorry, Adam—I'd love to do the PR, but the way I'm going at the moment, it might be a few weeks before I get the bandwidth 😔 If you're keen to get the next release out the door, I'm not sure I can contribute in time! — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe.

jimjam-slam commented 4 years ago

Okay! I'll aim for that then :)

maelle commented 1 year ago

From the README

This package has been archived due to BOM's ongoing unwillingness to allow programmatic access to their data and actively blocking any attempts made using this package or other similar efforts.