'as.Date' forces use of 'tz' and can handle 'tz' as a vector; closes …

[lsilvest]

…#118

I think forcing the use of 'tz' is essential; we didn't enforce it before, so it would revert to the default way that POSIXct infers timezone which is exactly what we set out not to do in nanotime because we wanted timezones to always be explicit. I think it reduces a lot the subtle errors that invariable creep in when functions automatically assume something about timezones.

The other improvement here of course is that 'tz' can now be a vector.

[eddelbuettel]

It's good and simple. I have one proposal though: would this be better?

edd@rob:~/git/nanotime(118-asdate-does-not-handle-the-case-where-tz-is-a-vector)$ git diff
diff --git a/R/nanotime.R b/R/nanotime.R
index 58c362e..494204a 100644
--- a/R/nanotime.R
+++ b/R/nanotime.R
@@ -339,6 +339,9 @@ setAs("nanotime", "POSIXlt", function(from) as.POSIXlt.nanotime(from))
 ##' @rdname nanotime
 as.Date.nanotime <- function(x, ...) {
     args <- list(...)
+    if (length(args) == 0) {
+        args <- list(tz="UTC")
+    }
     if (!("tz" %in% names(args))) {
         stop("'as.Date' call is missing mandatory timezone argument 'tz'")        
     }
diff --git a/inst/tinytest/test_nanotime.R b/inst/tinytest/test_nanotime.R
index 67fcf81..5d05eb1 100644
--- a/inst/tinytest/test_nanotime.R
+++ b/inst/tinytest/test_nanotime.R
@@ -352,7 +352,7 @@ nanotime_vec <- rep(nanotime("2023-09-21T22:00:00 America/New_York"), 2)
 tz_vec <- c("UTC", "America/New_York")
 expect_identical(as.Date(nanotime_vec, tz=tz_vec), as.Date(c("2023-09-22", "2023-09-21")))
 ## test missing argument 'tz':
-expect_error(as.Date(nanotime_vec), "'as.Date' call is missing mandatory timezone argument 'tz'")
+#expect_error(as.Date(nanotime_vec), "'as.Date' call is missing mandatory timezone argument 'tz'")
 
 ## c, subset, subassign and binds
 ##test_c <- function() {
diff --git a/tests/simpleTests.R b/tests/simpleTests.R
index 2bf5fdc..8d96dc0 100644
--- a/tests/simpleTests.R
+++ b/tests/simpleTests.R
@@ -34,6 +34,7 @@ as.POSIXct(x)
 as.POSIXct(x+1000)
 as.POSIXlt(x)
 as.POSIXlt(x+1000)
+as.Date(x)
 as.Date(x, tz="UTC")
 options("digits.secs"=od)
 
edd@rob:~/git/nanotime(118-asdate-does-not-handle-the-case-where-tz-is-a-vector)$ 

Passes all tests. Question is what we want nanotime(x) to be.

[eddelbuettel]

Nicely done.

[eddelbuettel]

The only thing I am wondering is ... that we are loosing an automatic, implied as.Date(x) where the UTC timezone is given.

[eddelbuettel]

because we wanted timezones to always be explicit.

I think I can live with that (as I can live with tz="UTC" being baked in. From nanotime to Date is a bit of a rare one anyway so I don't think we bite too many people.

[lsilvest]

Yes, I think you're right on this. By using a default of UTC, we have the same behavior as the generic as.Date with POSIXct and it's likely not a surprise then. We're also leaving the world of nanotime with as.Date; so defaulting to UTC is definitely better as you propose above.

[lsilvest]

nanotime(x) when x is a date is a great question too. We currently have this implementation:

setMethod("nanotime", "Date", function(from) nanotime(as.POSIXct(from)))

I think we should be forcing an argument tz to be specified here. And worse, in our current implementation, we don't even have an option to specify it. It's automatically and uniquely 'UTC'.

[eddelbuettel]

Happy to commit what I have into the branch.

We could possible use an options() slot and then do getOptions("nanotimeTz", "UTC") to default to UTC if nothing is set. [ Checks, then laughs. ] In fact we already do in a number of places 😀

[lsilvest]

Yes, that would be great if you could commit what you have.

Not so sure about the getOptions in this case because then we deviate from the generic as.Date. I think I prefer what you have with 'UTC' and like that it's the same behavior for the same function.

[lsilvest]

What are your thoughts on forcing the tz in the constructor from the Date type? I think at the very least we should be adding the tz argument. But that's one place where I could get my arm twisted to take it when missing from getOptions as it's what we do with nanotime for a character string that doesn't contain a timezone. Although in the character case I believe to get there one also has to change the default nanotimeFormat to not contain the timezone...

[eddelbuettel]

Not sure myself and we could delay that / decouple it from this PR.

[lsilvest]

Agreed, let's decouple.

[eddelbuettel]

I just pushed my changes from yesterday so now you can (informally, no button to click) my new changes to the PR.

[lsilvest]

I think line 343 could replace the stop. That is, if we don't find tz then we add it, so no exception. We could also potentially be stricter as the ellipsis for this case should either be no argument or tz, but other argument names are not supported, so we might want to raise an exception if the user calls as.Date(some_nanotime, foo=1).

[eddelbuettel]

I committed the above so do you want to just commit another refining iteration? When I wrote it was mostly as a 'how about ...' to raise an argument.

[eddelbuettel]

Nice!

[eddelbuettel]

Got distracted again but I think we are ready to merge as is. I jusst added a quick extension to ChangeLog (for the last bit) and NEWS (for this PR).

[lsilvest]

Yes, great, it looks all good, merging it.