-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for issue #327, adjust data request date and fix column name #328
Conversation
Merge in upstream
thanks @potterzot however, a few things:
|
Thanks @sckott. The file connections were not being closed properly during error checks, leaving behind temporary files. I think all of that is fixed now, and I've added a test to check the list of files in the temporary directory before testing against the list afterward and fail if they are not equal. It works on my linux system, but I'm not sure about other systems. I think it should work since I base the temp directory on the output of For the other test, I am confused about the |
thanks - i'll have a look |
thanks much @potterzot Files are now cleaned up on function exit, thats great, thanks. however, still seems like CRAN team may not be happy where files are being written. they say
on my macos, files are being written to |
Hmm, I'm not sure what is going on here. |
Okay, yeah they're not consistent probably in checking. We're probably okay as long as the files are not left anywhere on disk. if you don't get a response by monday or tuesday we should move on as we need to get it in by the end of next week |
I received an email from the Hopefully this is acceptable by CRAN, since it's not an R package that is doing the file creation and if we're careful to close file connections so that the temporary files get deleted afterward. If not, we may have to remove |
Just to follow up for future/posterity, I received this email regarding how to specify the tempfile location:
Since we'd have to create the
|
thanks @potterzot for looking into the netcdf file location details! Let's think about in future versions if we can do something about the temp files, but for now let's merge this, and submit to cran, and if they're still not happy then we can explore letting users change the location of the ncdf temp files, etc. |
Description
Two issues arose in the latest CRAN submission:
Sys.time()
with a forecast time of '0000'. As a result if the tests were run before '1800', the data file wouldn't be available yet and some tests would fail. This PR fixes that by changing the GEFS request to query a forecast made two days ago, which should always be available regardless of what time the tests are run.ens
andtime
dimensions, with the result that the request returned a data.frame withtime
as a column, whereas previously the specific requested variable was returned (eithertime1
ortime2
). The test still tested for variables under those names instead of under thetime
name. Not sure how this passed the last round of tests but all tests now pass.Related Issue
Fix #327