fix typo in check_states

[mpadge]

You guys have done a great job in changing the package in response to the reviews. I'm mighty impressed and already happy to sign off on this. I'm also getting on to the 9 & 15 bulletins, but first found this misplaced bracket. I also changed warning to message, because warning is perhaps a bit harsh in this context, and will cause potential failures in downstream dependencies, which seems perhaps unfair. Also note that current behaviour is still not ideal because multiple matches in .check_states result in no value being returned, and so:

> get_precise_forecast("s") # just trying to make it crash!
Multiple states match ...
Error in if (the_state != "AUS") { : argument is of length zero

Just a suggestion, but what I've done in bikedata is to standardise all cities to two-letter forms in convert_city_names. All that requires in anticipating every possibly two (or sometimes three) letter form any and all possible cities might start with - the vector of city_names - and then pmatch-ing those to standardised city-codes. Not meaning to blow my own trumpet here, but I do think that results in neater code because you only need to anticipate the first two letters, which also applies here ("vi", "ne", "ns", "ql", "qu", "no", "nt", ...). It's also fail-proof because it can only give a single value or an error.

[mpadge]

@adamhsparks @HughParsonage @Keith-Pembleton

you can ignore most of the above, because I've now added a provisional get_weather_bulletin() function. This needs state-dependent URLs, so I had to code up a convert_state() function anyway. This doesn't yet permit "AUS", but you could easily modify it for that. Notes on what I've done here:

1. You were indeed right is your suspicion that the data would be messy. Well, in fact the data themselves are no problem, but the bloody headers of the tables are a nightmare! Most of the code is the tidy_bulletin_header() function, which is admittedly really ... messy. A functional first cut only.
2. I've put in 2 more deps, but one is janitor which is very lightweight and likely useful for code you've already got (janitor::clean_names() is awesome!). The other is rvest, which is the easiest way by far to extract a html table, and is also reasonably lightweight. I hope that's okay!
3. I've put all importFrom instances in the roxygen comments, which you've not done throughout. I still don't understand why this passes R CMD check without them, but they really ought to all be there in the NAMESPACE. To keep things tidier and avoid repetition, another option is to put all importFrom calls together in your bomrang.r file.
4. I've just left the names of variables in generic janitor::clean_names() form, and shall leave it up to you to standardise these how you see fit.
5. I've not put minimal versions for janitor or rvest because there will be no min version - both are so new that any version will work

Let me know what you think. Note, however, that i'll be off on 3 weeks of holiday from this weekend. Bad timing for you guys, I know! I'll be able to respond in text form, but might not be able to do that much coding ...

Oh, and this is the only test code you need:

states <- c ("v", "nt", "q", "w", "s", "t")
for (s in states){
  message (nrow (get_weather_bulletin (s)))
  message (nrow (get_weather_bulletin (s, morning = FALSE)))
}

[adamhsparks]

Thanks @mpadge!

Re: importFrom, I'm happy to do that, but thought that when using the :: notation it wasn't necessary to state that. My other packages are written in this fashion with no errors or issues that I'm aware of.

It's easy enough to add this though. I'll have a poke around and see what more I can find about it.

No worries about the added dependencies. I figured rvest would be one already.

[HughParsonage]

Yes: you should either use importFrom or :: not both. On Fri, 25 Aug 2017 at 1:45 pm, Adam H. Sparks notifications@github.com wrote:

Thanks @mpadge https://github.com/mpadge!

Re: importFrom, I'm happy to do that, but thought that when using the :: notation it wasn't necessary to state that. My other packages are written in this fashion with no errors or issues that I'm aware of.

It's easy enough to add this though. I'll have a poke around and see what more I can find about it.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/ToowoombaTrio/bomrang/pull/54#issuecomment-324816559, or mute the thread https://github.com/notifications/unsubscribe-auth/AHvGDHZuh9585MviTVnfoWJ-MZc4qCROks5sbkNOgaJpZM4PBKC8 .

[adamhsparks]

Thanks @HughParsonage, I did remember correctly then.

I prefer to use the :: so that I know where a function came from in the package at a glance.

[mpadge]

Ah interesting, so that's my mis-reading of things because i always use both. I'll have to reconfigure my own practice then ... :+1: