Closed jonocarroll closed 5 years ago
One option would be to also have a header = TRUE
option (default on).
(now with even more correct spelling)
Nice! I'm polishing the nasapower package right now, will get to this PR soon.
Seeing these errors when checking:
Undocumented code objects:
‘arrange.bomrang_tbl’ ‘filter.bomrang_tbl’ ‘group_by.bomrang_tbl’
‘mutate.bomrang_tbl’ ‘rename.bomrang_tbl’ ‘select.bomrang_tbl’
‘slice.bomrang_tbl’
All user-level objects in a package should have documentation entries.
@jonocarroll, can you address them?
Yeah - I was planning to inherit the dplyr documentation as these are merely attribute-preserving wrappers.
If you're comfortable with my implementation I'll start adding checks/tests.
On Wed, 26 Sep. 2018, 9:01 pm Adam H. Sparks, notifications@github.com wrote:
Seeing these errors when checking:
Undocumented code objects: ‘arrange.bomrang_tbl’ ‘filter.bomrang_tbl’ ‘group_by.bomrang_tbl’ ‘mutate.bomrang_tbl’ ‘rename.bomrang_tbl’ ‘select.bomrang_tbl’ ‘slice.bomrang_tbl’ All user-level objects in a package should have documentation entries.
@jonocarroll https://github.com/jonocarroll, can you address them?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/bomrang/pull/91#issuecomment-424681744, or mute the thread https://github.com/notifications/unsubscribe-auth/AJDpIXNQgKmaddQAGQ22V4szym4FkGKhks5ue2WBgaJpZM4W1e_D .
Related to my last comment about data.table
vs a data.frame
. Looking at this again, is it worth just using the bomrang_tbl across the package for consistency?
Maybe we should employ the same print methods for all of the returns in bomrang
? Looking at the XML files for the forecasts there are metadata in them that we're currently ignoring, e.g.,
<identifier>IDN11001</identifier>
<issue-time-utc>2018-10-04T18:10:00Z</issue-time-utc>
<issue-time-local tz="EST">2018-10-05T04:10:00+10:00</issue-time-local><sent-time>2018-10-04T18:10:08Z</sent-time>
<expiry-time>2018-10-05T18:10:00Z</expiry-time>
<validity-bgn-time-local tz="EST">2018-10-05T00:00:00+10:00</validity-bgn-time-local>
<validity-end-time-local tz="EDT">2018-10-07T23:59:59+11:00</validity-end-time-local>
<next-routine-issue-time-utc>2018-10-05T06:05:00Z</next-routine-issue-time-utc>
<next-routine-issue-time-local tz="EST">2018-10-05T16:05:00+10:00</next-routine-issue-time-local>
Could be nice to include this information in headers and have a uniform print method for all items returned by bomrang
.
@jonocarroll, I'm good with this if you want to polish it and get it ready for me to accept.
I think we'll use a custom print function for all the data frames in bomrang
so that it's uniform across functions and data returned. Some might benefit from the metadata header, others won't but the data.table
print method is nice.
I've made some other changes to the package, fixing documentation, making sure it complies with CRAN policies and handling data downloads more gracefully. I will work on https://github.com/ropensci/bomrang/issues/88 shortly to get an image in when an error occurs so we can get a new release out with these fixes.
You're in luck - I have another long flight coming up next week 🙃
My plan was to use this function as a test of how the classed print might look then roll it out to the other objects in the package. I'll clean up what I have and continue developing as best as I can. I did get stuck re-exporting the dplyr
verbs... well, all except filter
are fine being classed. I'll keep at it.
I think I've got tests in place and a few other things I haven't yet pushed because I broke filter
, but I'll push these too. If you don't mind waiting a bit, this could be really very tidy.
I don't mind waiting a bit. It'll take me a while to get the image stuff squared away.
Right, I think this works okay now. I can't seem to test it from the airport lounge so apologies if it's actually broken. Take it for a spin and see what you think. I can then see what I need to do to generalise it to another response (please suggest which to try).
Functions nicely when I tested it yesterday.
Regarding implementing with other BOM data. I was looking at the XML files for the ag bulletins, e.g. ftp://ftp2.bom.gov.au/anon/gen/fwo/IDN65176.xml. We could include some of the information from the beginning of the XML file that suggests looking at the information at the bottom of this page: http://www.bom.gov.au/cgi-bin/wrap_fwo.pl?IDN65176.html in the header when printed in the console.
Otherwise, just using a standard print method for all data returned, even if no additional information is passed along in the header would be good.
I've expanded the class to work with get_current_weather
also, so you can see what it involves. So far not much apart from having the station info defined. Let me know what you think of this and I'll keep iterating.
The failing tests are unrelated. I brought this branch up to date with develop and all my functions pass.
I've updated the internal databases and fixed tests accordingly for the updated data.
However, line 303 of get_current_weather(), count = format(diff(range(x$local_date_time_full)), digits = 3)
errors, I can't retrieve current weather due to x
not being found, Error in diff(range(x$local_date_time_full)) : object 'x' not found
.
I do like what I'm seeing though! Fantastic work!
My bad - had a test value in (thought I tested but apparently not). I also repaired some of the other tests for get_current_weather
which would have failed too. I think it's working for those two now.
FYI I've bypassed the 'raw' output for get_current_weather
which may not be ideal. Do you still want that option? I'm also using data.table
to output by default, so do you want to keep that option?
Yeah, not ideal to bypass the raw
parameter. I think we need to make sure all the columns are returned as the proper class in all functions.
We can just keep data.table
outputs for everything, I think. I/we just need to check and update documentation where appropriate.
(it was a long flight)
Initial pass at S3 headers for
get_historical()
.Created on 2018-09-23 by the reprex package (v0.2.0).
It looks a bit better in a terminal/RStudio, rather than GitHub's fixed-width window.
Thoughts? I've left it flexible enough that the
bomrang_tbl
class could be attached to results from other queries and processed similarly.