Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

fix #133 #134

Closed
wants to merge 1 commit into from
Closed

fix #133 #134

wants to merge 1 commit into from

Conversation

mpadge
Copy link
Collaborator

@mpadge mpadge commented Mar 25, 2021

This should fix #133. Changes were manifold, due to both tibble updates breaking some of the sub-settting, and rvest updates which greatly improved extraction of the messy tables, so former assumptions on structure of return values no longer applied.

The PR happily removes most of the formerly messy code individually addressing individual rows and columns of the tables, and reduces the tidy_bulletin_header fn down to two simple sub-functions. Much nicer!

Confirmation that all works:

setwd ("/data/mega/code/forks/bomrang")
devtools::load_all (".", export_all = FALSE)
#> ℹ Loading bomrang
#> Registered S3 method overwritten by 'hoardr':
#>   method           from
#>   print.cache_info httr

states <- c ("vic", "nsw", "qld", "nt", "tas", "wa", "sa")
vapply (states, function (i)
        nrow (get_weather_bulletin (i, morning = TRUE)),
        integer (1))
#> vic nsw qld  nt tas  wa  sa 
#>  93 188 134  56  49 136  90
vapply (states, function (i)
        nrow (get_weather_bulletin (i, morning = FALSE)),
        integer (1))
#> vic nsw qld  nt tas  wa  sa 
#>  82 140 132  53  39 132  71

Created on 2021-03-25 by the reprex package (v1.0.0)

@mpadge mpadge requested a review from adamhsparks March 25, 2021 10:39
@adamhsparks
Copy link
Collaborator

Thanks, @mpadge. I poked at it a little bit yesterday but wasn't familiar enough to make these changes so quickly!

I'm working on it now, modifying the function slightly to use curl_download() to take advantage of curl's options with the custom handle since I was getting a timeout with this function and I've updated the test to check the new colnames.

@mpadge
Copy link
Collaborator Author

mpadge commented Mar 26, 2021

Have you tried using crul for async requests? I suspect that might be more straightforward than processing curl_download results when things go wrong? Just a thought in case that helps

@adamhsparks
Copy link
Collaborator

Forgive my ignorance here, I've used crul where I actually have a proper API to talk to, nasapower, but I don't understand what you're fixing by using it here?

@mpadge
Copy link
Collaborator Author

mpadge commented Mar 26, 2021

Forgive my ignorance here, I've used crul where I actually have a proper API to talk to, nasapower, but I don't understand what you're fixing by using it here?

Sorry, only because you mentioned timeout issues which it can help with. But I'm sure you've got a better idea than me here, so please ignore and continue as you were, good sir!

@adamhsparks
Copy link
Collaborator

adamhsparks commented Mar 26, 2021 via email

@adamhsparks
Copy link
Collaborator

I merged this locally with the devel branch. Thanks again for the quick response, @mpadge.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get_weather_bulletins() fails
2 participants